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