Hi Bruno,
On 3/31/24 5:12 AM, Bruno Haible wrote:
> 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.
That makes more sense now that I think about it. Thanks!
> 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.
I'm not sure where I got in the habit of using '@property' from. I
tend to agree with other decorators though. I find them to be
confusing much of the time [1]. But maybe there are other situations
where I would not mind them.
> 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).
Yes that makes sense.
That reminds me though, it would be nice to decide on a naming
convention at some point. Here is a few examples from
GLModuleSystem.py, though other files have similar occurrences:
1. GLModuleSystem.file_is_module()
2. GLModule.isTests()
3. GLModule.getAutomakeSnippet_Unconditional()
4. GLModuleTable.getCondition()
5. GLModuleTable.transitive_closure()
Our class names are fine, but it would be nice for the function names
to agree with each other.
I prefer the naming convention of 1 and 5, which is more standard PEP8
[2]. It also is close to what we use in gnulib-tool.sh:
GLModuleTable.transitive_closure -> func_modules_transitive_closure
The naming convention of 2 and 4 reminds me of Java. Maybe if I used
that language more I would like the convention. I would be willing to
tolerate it for the sake of consistency. :)
No rush of course, but now that test cases are *almost* all passing,
we can think about cleaning up a little bit.
> 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.
You're welcome. Thanks for all of the feedback. And patience with some
of my large, unorganized patches. I don't have much experience
contributing, including other projects, so I am still learning.
> - 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.'''
I will probably steal this style for other docstrings. I find it
easier to reference with the arguments listed individually as opposed
to a single paragraph.
[1] https://stackoverflow.com/a/10300995/23476799
[2] https://peps.python.org/pep-0008/#function-and-variable-names
Collin
