Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.

[Collin Funk]

Hey Bruno, thanks for the feedback.

On 2/23/24 5:08 AM, Bruno Haible wrote:
> Just three small nits that you might want to revisit:
> 
> * gnulib-tool.py line 610:
> +    if modules != None and "tests" in mode and gnu_make:
>         ^^^^^^^^^^^^^^^^^^^
> What is the rationale for this condition? It is not present in the original. 
> Can
> it be removed?

I added this condition in an attempt to mirror changes made in commit
60e8b9303d8ce312bb2322d4801ed08678f93d1e which was marked in 
gnulib-tool.py.TODO. It seems correct to me. Here is the original
change from gnulib-tool. I am not the best when it comes to
reading/writing shell scripts so I could always be wrong. :)

* gnulib-tool Line 1493:
+  case $mode,$gnu_make in
+    *test*,true)
+      echo "gnulib-tool: --gnu-make not supported when including tests"
+      func_exit 1;;
+  esac

The "mode" variable is initialized to None and then set in the
conditionals before the one I added depending on the arguments. Since
it may still be None it must be checked before treating it as a string
to avoid an uncaught exception.

> * pygnulib/GLEmiter.py lines 732, 739:
> +                        allsnippets += re.sub(r'^if (.*)', r'ifneq 
> (,$(\1))', amsnippet1)
> +                        allsnippets += re.sub(r'^if (.*)', r'ifneq 
> (,$(\1))', amsnippet2)
> 
> The regular expression is duplicated. The problem with duplicated code is
> that it very often leads to bugs in the future: Future code maintainers
> will see one copy of the piece of code, modify it, and leave the other
> copy unchanged. Sometimes it's a bug because it's inconsistent; sometimes it's
> a bug because the other copy lacks the feature for which the modification
> was made in the first copy.
> 
> So, the lesson is: Try hard to avoid code duplication!

I agree. I will have to revisit that code a few times to handle the
other items in the TODO. I'll try to think of a good way to clean that
up. An idea that makes sense to me currently is storing the regular
expressions in the "convert_to_gnu_make" variable as a tuple/list if
"gnu_make" is set. If it is not set, the variable can be set to None.
This would essentially let it act as a boolean:

if convert_to_gnu_make != None:
   do something

But would also allow it to be used as the regular expression
arguments:

allsnippets += re.sub(convert_to_gnu_make[0],
                      convert_to_gnu_make[1], amsnippet1)

That is just an idea that I have now. I might discover something
better later down the line. In the future it would be nice to cleanup
many different parts of the code. One thing that annoys me personally
is comparing to none using "!=" instead of "is not". This is
recommended against in PEP 8, "Comparisons to singletons like None
should always be done with is or is not, never the equality
operators." [1]. I also am a big fan of using Pathlib for portable,
high-level path construction [2]. Currently we use a few functions
wrapping the os.path versions which I find less readable. Larger
changes like this are probably best left until gnulib-tool.py catches
up in features to gnulib-tool though. They would probably make it
harder to follow the git history.

> * pygnulib/GLConfig.py line 802:
> Typo: Autmake -> Automake.

Oops. I also noticed that I forgot the remove the --gnu-make option
from the missing arguments gnulib-tool.py.TODO. It seems that you
remembered for me. Thanks!

[1] https://peps.python.org/pep-0008/#programming-recommendations
[2] https://docs.python.org/3/library/pathlib.html

Collin