[Thuban-list] Patch: classifiers
bh at intevation.de
Thu Feb 12 21:14:17 CET 2004
"Daniel Calvelo Aros" <dcalvelo at minag.gob.pe> writes:
> Hi all.
> The attached patch provides four classifiers to thuban's available three.
Thanks. It would be nice to have that functionality in Thuban. Before
we check it in, though, I'd like to see some improvements in the code.
First some stylistic things. To make the code more maintainable in the
context of Thuban and to make it easier for us to review the patch,
please try to follow the style and practices used in the code already
there. These might also serve as the beginning of a "patch submission
- All lines should be at most 79 characters long
- Put spaces around binary operators and after commas, colons and
semicolons (but not before them).
- Every class, function and method should have a doc string. The doc
string should start with a brief one-line description of the class or
method. If more explanations are needed add an empty line and one or
The isn't adhered to in the code as well as it should. New code
should follow it, though.
- New functionality and bug fixes should have corresponding tests in
the test suite.
- Changes should be documented in the ChangeLog file
- Do not use "from module import *"
This form of the import statement leads to code that is hard to
maintain for several reasons:
- If the module's contents change the names bound in the code that
executes the import statement change as well and might
accidentally override python builtins or names already bound in
- It's hard to find out which of the objects in the imported module
are actually used by the importing code. It's especially hard to
find out whether the import is still needed if the code has
"from wxPython.wx import *" is tolerable, but I try to avoid that too.
- Try to submit small patches. Smaller patches are easier to
understand, take less time to review and are therefore much more
likely to be applied quickly. That you submitted the HSV support as
a separate patch first was a good example of this.
More specific comments on the code
- Thuban/Model/color.py: rgb2hsv and hsv2rgb duplicate functionality
from Python's colorsys module
- Thuban/Model/classgen.py: green_to_red_ramp defines a different ramp
now. This is an incompatible interface change. If we really do need
a predefined ramp for this it should be called something else,
Ideally all these predefined ramps don't belong in Thuban.Model
anyway. Currently they would be better in Thuban.UI. But that would
have to wait until create the branch in CVS as that kind of change
doesn't belong in a stable branch.
- Thuban/Model/classgen.py: Line 18f: The new import statements are not
necessary. sqrt and random are never used. I guess they were
required by an earlier version of your code
- Thuban/Model/classgen.py: Renaming the __SetProperty method is OK. I
suggest to rename it to interpolate_colors and have it return the new
color instead of passing the setf function in. E.g.
def interpolate_colors(self, color1, color2, weigth):
"""Return a weighted average of the colors color1 and color2
The returned color is effectively
(1 - weight) * color1 + weight * color2
weight should be a float in the range 0.0 to 1.0
Come to think of it, it's probably be even better to put that method
as a function into Thuban/Model/color.py and add an optional
parameter to CustomRamp defaulting to that new function. The
_SetProperty implementation in HSVCustomRamp could be another
function in colors.py, perhaps interpolate_colors_hsv (and the other
should get an _rgb suffix, then). There'd be no need for the new classes
HSVCustomRamp and HSVRamp, even.
- Thuban/UI/sliders.py: Line 11: Yes, it should use a better way to
find the bitmap file. That's what the functions in
Thuban/UI/resource.py are for.
- Thuban/UI/classgen.py: The sum function is new as a built-in in
Python 2.3. Thuban 1.0.x should remain compatible with Python 2.2.
Thuban 1.1.x will probably also remain 2.2 compatible.
I haven't looked at all of the code yet, but this should be enough for
Intevation GmbH http://intevation.de/
More information about the Thuban-list