Re: [gnulib-tool-py] Modules and avoids: migrate to GNULibMode class

[Bruno Haible]

Hi Dmitriy,

You have immediately spotted the weak points in my proposal :-)

> 1) Some module names are too long and too cryptic at the same time. The
> second problem can be solved in the docstrings, but the first is a real
> headache. It conflicts with Zen of Python (to read it type in interpreter:
> import this).

These are wise, guidelines with a lot of experience behind them, indeed.

GNULibReadOnlyOperations could be split into two:

1a) class GNULibFileSystem
    implements func_lookup_file

1b) class GNULibModuleSystem
    subclass of GNULibFileSystem
    implements
     func_exists_module
     func_verify_module
     func_verify_nontests_module
     func_verify_tests_module
     func_get_description
     func_get_comment
     func_get_status
     func_get_notice
     func_get_applicability
     func_get_filelist
     func_filter_filelist
     func_get_dependencies
     func_get_autoconf_early_snippet
     func_get_autoconf_snippet
     func_get_automake_snippet_conditional
     func_get_automake_snippet_unconditional
     func_get_automake_snippet
     func_get_include_directive
     func_get_link_directive
     func_get_license_raw
     func_get_license
     func_get_maintainer

2) class GNULibModuleList
   - renamed from class GNULibModuleListOperations

The others are short enough.

> How do you think, wouldn't it be better
> to change names of the classes from GNULib* to GL*?

May be a good idea, yes. Your choice.

The list of class names would now be
  GLFileSystem
  GLModuleSystem
  GLModuleList
  GLGenerator
  GLImport
  GLTestDir
  ...


> > class GNULibReadOnlyOperations
> >
> Here is my proposal how to change this class.
> We create new class GNULibModule, which inherits GNULibMode. This class has
> only one additional argument in __init__: module name. When we create new
> GNULibModule instance, it checks module (analogue of the
> func_verify_module) and if all is great it creates instance.
> Inside GNULibModule it we have all methods which need only methods which
> need basic attributes from GNULibMode and the name of the package. To get
> the type of the module, we simply use methods. Here is an example:
> 
> module = GNULibModule(destdir, localdir, name)
> module.isTest() # returns True or False
> module.getDescription() # returns description of the module
> module.get*() # returns * of the module

Good point, this moves the entire design to a more object-oriented direction.

Just some thoughts:
  - The 'destdir' is inappropriate here. We need a destdir only when
    writing to disk.
  - 'localdir' is a bit too concrete. I would like to see the construction
    take a GNULibFileSystem object:
       module = GNULibModule(filesystem, name)
  - GNULibModule should not inherit from GNULibMode or GNULibModuleSystem,
    it needs to encapsulate a [pointer to] an instance of a GNULibModuleSystem.
    Why? Because the GNULibModuleSystem contains the modcache, and a cache
    is not efficient if each GNULibModule has its separate cache. The cache
    must be shared among the various GNULibModule instances.
  - You also need to think about whether two GNULibModule with the same
    module name should be able to exist. If yes, then you need to define
    an override for __eq__. If no, then the constructor must take a
    GNULibModuleSystem as argument (not merely a GNULibFileSystem), and
    must return a previous instance if that already existed.

> It seems we don't need to verify module using func_verify_module, because
> it is already checked when user creates GNULibModule instance. I suppose we
> only have to check module in __init__ part and catch GNULibError #3 if we
> need.

Yup, I agree.

> And I suggest to rewrite addModule, addAvoid, setModules, setAvoids, etc.
> to accept instances of the GNULibModule instead of strings.

This is a possibility. I can't say at this point whether it would be
beneficial.

> As for func_lookup_file, it is done where it is needed (in our case
> GNULibImport class) and returns correct module names, from which we then
> create GNULibModule instances.

I don't understand what you mean here.

> > class GNULibModuleListOperations
> >
> All is right, however, I suggest to cut name to GNULibModuleList.

Sure.

> I think that it must inherit not from GNULibModule, but from GNULibMode since
> GNULibModuleList has nothing common with GNULibModule class.

Right. In my proposal, it would inherit from GNULibModuleSystem or (if
you decide that there is no GNULibModuleSystem) from GNULibFileSystem.

> func_acceptable will be deprecated since it just tests whether module is in
> the self.args['avoids'] list. We don't need function for this, because we
> can just use such construction: return(module in self.args['avoids']). It
> returns True or False. That's pretty enough.

Sure. Some things are easier in Python than in shell scripts. This is
one of them.

> func_get_tests_module will be deprecated too, since we have just to use
> something like this:
> 
> try: # Try to create testsmodule for module
>   testsmodule = GNULibModule(destdir, localdir, module)
> except GNULibError as error:
>   if error.errno == 3:
>     print('There is no tests module for module %s' % error.errinfo)

Good, you have already reduce this function a lot. But 5 lines of code
vs. a function call - I would prefer the function call.

> As for func_module_shellfunc_name, func_module_shellvar_name,
> func_module_conditional_name and similar functions, where each function
> needs only module name... I think that it fits best to GNULibModule: we
> just have to create methods for each such function.

Hmm. func_module_shellfunc_name also depends on the macro_prefix.

Bruno