Hi Collin,
> Not sure why I didn't do this in the first place to be honest...
> Here is two patches. Most of the change is in these lines:
>
> if self.exists(module):
> path, istemp = self.filesystem.lookup(joinpath('modules',
> module))
> - result = GLModule(self.config, path, istemp)
> + # Get the directory containing the module. May be a
> '--local-dir' directory.
> + directory = remove_trailing_slashes(subend(joinpath('modules',
> module), '', path))
> + result = GLModule(self.config, directory, module, istemp)
> return result
>
> So, as you explained in your example, we remove 'modules/NAME' from
> 'gnulib_dir/modules/NAME' to get the directory. Then we pass that
> directory and the module name to GLModule.__init__().
Thanks; this is what I proposed.
But when I look at your patch now, I see that it can be done in an even simpler
way:
- From the path and the module name, you compute the directory.
The only use of that directory is to compute the path again.
- This directory _may_ be from a --local-dir option, but it may
also be a temporary directory (cf. GLFileSystem.py:135:
result = (tempFile, True)
- So, if we pass the name and the path, we don't need any further
computations.
> Also, I used the '@property' decorator for getting the path. Maybe
> this is personal preference, but I like it for values that should be
> read only [1]:
>
> + @property
> + def path(self) -> str:
> + '''Return the path to the module description file.'''
> + return joinpath(self.directory, 'modules', self.name)
To me, this '@property' decorator is just meaningless meta-clutter:
- The documentation makes it sound like if you use @property,
you also need to provide a setter and deletor function.
- The code does not make use of the 3 methods as a combined property.
To indicate a read-only property, just define a 'getPath' method, and
hide the 'path' field by prepending an underscore: '_path'.
The getPath / setPath method naming for a property named 'path' is
well-known across programming languages (other than Lisp).
I'm therefore applying the patch below instead.
> The second patch just fixes the Coreutils --local-dir module sorting
> by changing __eq__() and friends to use the module name and not the
> path.
Thanks, I'm applying it.
Thanks, btw, for having sent this as two patches. It helps the code
review. It also allowed me to provide a simpler alternative for the first
part, without touching the second one.
Bruno
2024-03-31 Bruno Haible <br...@clisp.org>
gnulib-tool.py: Make a module's name immediately accessible.
* pygnulib/GLModuleSystem.py (GLModuleSystem.find): Pass the module name
to the GLModule constructor.
(GLModule.__init__): Accept the module's name as argument and store it.
(GLModule.getName): Simplify.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 4add195f2a..0852d1dfa9 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -102,7 +102,7 @@ class GLModuleSystem(object):
% type(module).__name__)
if self.exists(module):
path, istemp = self.filesystem.lookup(joinpath('modules', module))
- result = GLModule(self.config, path, istemp)
+ result = GLModule(self.config, module, path, istemp)
return result
else: # if not self.exists(module)
if self.config['errors']:
@@ -183,22 +183,27 @@ class GLModule(object):
# List of characters allowed in shell identifiers.
shell_id_chars =
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_'
- def __init__(self, config: GLConfig, path: str, patched: bool = False) ->
None:
- '''Create new GLModule instance. Arguments are path and patched, where
- path is a string representing the path to the module and patched is a
- bool indicating that module was created after applying patch.'''
+ def __init__(self, config: GLConfig, name: str, path: str, patched: bool =
False) -> None:
+ '''Create new GLModule instance. Arguments are:
+ - name, the name of the module,
+ - path, the file name of the (possibly patched) module description,
+ - patched, indicating whether that module description was created by
applying a patch.'''
self.args = dict()
self.cache = dict()
self.content = ''
if type(config) is not GLConfig:
raise TypeError('config must be a GLConfig, not %s'
% type(config).__name__)
+ if type(name) is not str:
+ raise TypeError('name must be a string, not %s'
+ % type(name).__name__)
if type(path) is not str:
raise TypeError('path must be a string, not %s'
% type(path).__name__)
if type(patched) is not bool:
raise TypeError('patched must be a bool, not %s'
% type(patched).__name__)
+ self.name = name
self.path = path
self.patched = patched
self.config = config
@@ -284,9 +289,7 @@ class GLModule(object):
def getName(self) -> str:
'''Return the name of the module.'''
- pattern = re.compile(joinpath('modules', '(.*)$'))
- result = pattern.findall(self.path)[0]
- return result
+ return self.name
def isPatched(self) -> bool:
'''Check whether module was created after applying patch.'''
