[Thuban-devel] Re: Patch for adding version info for Extensions

[Jan-Oliver Wagner]

Hi,

thanks to Bernhard Herzog for the comments.
I considered most, but have two quetions remaining.

On Wed, Aug 04, 2004 at 08:25:24PM +0200, Bernhard Herzog wrote:
> Jan-Oliver Wagner <jan at intevation.de> writes:
> > I decided to have a explicit registration for the extensions.
> >
> > Let me know what you think.
> >
> > Attached is a patch (based on current CVS HEAD) and a new
> > file Thuban/UI/extensionregistry.py
> [...]
> > class ThubanExtensionDesc:
> 
> I'd omit the Thuban prefix from that name.  If ExtensionDesc is too
> ambigious in the code that imports this, that importing code can always
> use import ... as... to choose a different name.  This applies to
> ThubanExtensionRegistry, too.

Done.

> >         min_thuban_versions -- Minimum Thuban versions required to
> >                                run the extension. This is a dictionary
> >                                where the major release numbers ('X.Y')
> >                                are the keys and the minor release
> >                                number is the contents (e.g. 'Z').
> >                                Example: { '1.0' : '0' }
> 
> I asked Jan yesterday why he chose this representation.  His goal was to
> make it possible for an extension to state that it can use e.g. 1.0.1
> and later from the 1.0 branch and 1.1.2 from the 1.1 branch.  That is,
> neither 1.0.0 nor 1.1.1 would work, but 1.0.3 and 1.1.5 would.
> 
> I have two issues with this:
> 
> 1. Should we support it at all?
> 
> We only need this if we backport features from newer Thuban versions to
> an older one.  Currently this would mean that we backport from cvs HEAD
> to the 1.0 branch.  
> 
> 2. Is this the right representation for the interface of ThubanExtensionDesc?
> 
> I think it's too complicated.  I suggest the following, using the same
> versions as in my example above:
> 
>    [(1, 0, 1), (1, 1, 2)]
> 
> That, is a simple list of versions, with each version a tuple of
> numbers.  Representing a single version as a tuple of numbers has the
> advantage that we don't have to parse the version numbers anymore and
> comparisons are done numerically.

I opt for 1. and would like to remove this part.
Its more like a solution looking for a problem.
Better lets wait until we face a real problem.
Ok to drop this again?

> >     def Get(self):
> >         return self.registry
> 
> I'd lose the Get method and implement some others instead which match
> the usage of the registry better.  See below.

See below.

> Another thing: I think the Add method should be spelled add, that is all
> lowercase.  The main reason is that I think we should, in the long term,
> adopt the more common naming styles used in python code.  For Thuban
> this basically means not to start method names with upper case letters.
> For Thuban this would be a substantial change and even though it would
> be easy to retain the old method names for a while for backwards
> compatibility it's not something that should be done soon.  OTOH, for
> small new classes such as this or the ClassMapper it wouldn't be a
> problem to adopt that naming scheme and, in fact, ClassMapper already
> uses it.

I've renamed Add to add and added your text to the coding guidelines
in CVS.

> > +ext_registry.Add(ThubanExtensionDesc(
> > +    name = 'gns2shp',
> > +    version = '1.0.0',
> > +    authors= [ 'Jan-Oliver Wagner' ],
> > +    copyright = '2003, 2004 Intevation GmbH',
> > +    desc = _("Converts GNS (Geographical Name Service\n" \
> > +             "of NIMA) to Shapefile format and\n" \
> > +             "displays the data."),
> > +    min_thuban_versions = { '1.0' : '0' }))
> 
> That min_thuban_versions is wrong.  1.0.0 doesn't have the extension
> registry :) The same applies to the other places where you call
> ThubanExtensionDesc.

See above. I opt to remove min_thuban_versions.

> > --- Thuban/UI/about.py	26 Mar 2004 18:15:35 -0000	1.12
> > +++ Thuban/UI/about.py	29 Jul 2004 12:27:14 -0000
> > @@ -70,6 +72,14 @@ class About(wxDialog):
> >              text+= '\t%s %s\n' % (name, version)
> >          text += '\n'
> >  
> > +        text += _('Extensions:\n')
> > +        if len(ext_registry.Get()) == 0:
> 
> I think we should implement either __len__ or __nonzero__ in
> ThubanExtensionRegistry so that you could write
> 
>  if ext_registry:
> 
> instead.

Done (__nonzero__)

> > +            text += _('\tNone registered.\n')
> > +        else:
> > +            for ext in ext_registry.Get():
> 
> And this would be easier written as 
> 
>     for ext in ext_registry:
> 
> by implementing a suitable iter method as e.g. a generator.

can you give me an example?

	Jan
-- 
Jan-Oliver Wagner               http://intevation.de/~jan/

Intevation GmbH                      http://intevation.de/
FreeGIS                                http://freegis.org/