Hi Collin, > > * 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.
I.e. you meant to write mode != None not modules != None ? > > 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) Sounds good. > 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 recall having seen this warning. Was it from python itself? Or from pycodestyle or pylint? (Cf. the comments at gnulib-tool.py line 29..33) But I don't recall whether this warning was justified or bogus. > 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. Yes, larger style changes like this one are probably best postponed for the moment. And I'm not convinced Pathlib is a win here, because - we are not targetting native Windows, only Unix platforms (that includes Cygwin), - it appears to have some extra learning curve besides 'import os'. Bruno