[Thuban-list] Patch: classifiers

[Bernhard Herzog]

"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
guideline".

 - 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
   more paragraphs.
   
   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
      the module

    - 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
      changed.

   "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,
   perhaps hsv_green_to_red_ramp.  

   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
now :)

   Bernhard

-- 
Intevation GmbH                                 http://intevation.de/
Skencil                                http://sketch.sourceforge.net/
Thuban                                  http://thuban.intevation.org/