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

[Bruno Haible <bruno@clisp.org>]

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