Comment 2 for bug 714731
Revision history for this message
| Louis Simard (louis-simard-deactivatedaccount) wrote | #2 |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
35a32c5
(Get the code!)
That code gives me deja vu for some reason, as if I had read it somewhere else already...
Anyway, on to the comments:
1. Are you in an environment where you can't make unified diffs, or perhaps you don't know how to make them yet? Unified diffs are an excellent way to make patches, where the diff itself shows what was removed and added, yet any contributor only needs to keep the old or new version of a file to apply or revert a patch. On Unix or BSD, you can copy the Bazaar repository version of the file to scour.py.old and issue "diff -u scour.py.old scour.py", after editing scour.py, to make a patch. On Windows you can probably get a tool to make unified diffs too, like a gnuwin32 port of 'diff'.
2. _getStyle and _setStyle could probably be used elsewhere when the code needs to read and write style attributes; well done on making the accessor code into functions.
3. Using str(), float() and convertColor() as canonicalising functions before checking the values is a nice touch, but it may make Scour a bit inefficient. It makes #000 equivalent to black, etc., but the time taken to ensure that both colors/numbers are canonical even after colors and number precision were optimised seems like a bit of a waste.
Overall your code looks sound, but I would hugely prefer having a patch in unified diff format to work with.