Hi Collin,
> I've implemented the initial support for --automake-subdir in
> gnulib-tool.py. It is a larger patch so let me know if you have any
> questions.
Thanks.
Though, it would be nice to improve a couple of things:
1) This form of conditional expression
base = self.config['destdir'] if self.config['destdir'] else '.'
mentions first a value, then a condition, then another value.
It would be good to mention the condition first, like in C, Lisp, Java, etc.
- even if that means that this needs 4 lines of code instead of 1 line.
We are not forced to use ill-designed language constructs. I'm adding
a coding style update, see below.
2) automake_options = {x for y in automake_options for x in y.split()}
Can you please reformat this as
automake_options = { x
for y in automake_options
for x in y.split() }
It is more readable this way.
3) Method GLConfig.setAutomakeSubdir claims that
"automake_subdir must be a string"
but it should be bool.
4) In GLEmiter.initmacro_end, you translated the condition
if $automake_subdir && ! "$2" && test -n "$sourcebase" && test "$sourcebase"
!= '.'; then
to
if automake_subdir and not gentests and sourcebase != '.':
What about the case sourcebase == '' ? We don't want subdir to be '/' in
this case.
(Or can this not happen?)
5) GLEmiter.shellvars_init has no function comment. How about
def shellvars_init(self, gentests: bool, base: str) -> str:
'''emits some shell variable assignments'''
Also please don't omit the comments that describe what gl_source_base and
gl_source_base_prefix. Because these are really hard to grasp without a
comment.
With all these little things to revisit, can you resend a patch with these
things fixed, please?
Bruno
2024-03-10 Bruno Haible <[email protected]>
gnulib-tool.py: One more comment regarding coding style.
* pygnulib/main.py: Add comment regarding conditional expressions.
diff --git a/pygnulib/main.py b/pygnulib/main.py
index 58d4cd268e..97ff76957a 100644
--- a/pygnulib/main.py
+++ b/pygnulib/main.py
@@ -34,6 +34,9 @@
# Use 'is' and 'is not' for variables which can contain only class instances
# (e.g. GLModule instances) and None.
# Rationale:
<https://lists.gnu.org/archive/html/bug-gnulib/2024-02/msg00225.html>
+# - Avoid conditional expressions as in PEP 308
<https://peps.python.org/pep-0308/>.
+# Rationale: They violate the principle that in conditional code, the
+# condition comes first.
# You can use this command to check the style:
# $ pycodestyle *.py
