[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.
From: |
Bruno Haible |
Subject: |
Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27. |
Date: |
Sat, 24 Feb 2024 00:51:43 +0100 |
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
- [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27., Collin Funk, 2024/02/23
- Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27., Bruno Haible, 2024/02/23
- Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27., Collin Funk, 2024/02/23
- Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.,
Bruno Haible <=
- Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27., Collin Funk, 2024/02/23
- gnulib-tool.py: Follow gnulib-tool changes, part 28., Collin Funk, 2024/02/24
- Re: gnulib-tool.py: Follow gnulib-tool changes, part 27., Bruno Haible, 2024/02/24
- Re: gnulib-tool.py: Follow gnulib-tool changes, part 27., Dima Pasechnik, 2024/02/24
- Re: Python != None, Bruno Haible, 2024/02/25
- Re: Python != None, Collin Funk, 2024/02/25
- Re: Python != None, Collin Funk, 2024/02/25
- Re: pycodestyle configuration, Bruno Haible, 2024/02/26
- Re: pycodestyle configuration, Collin Funk, 2024/02/26
- Re: pycodestyle configuration, Bruno Haible, 2024/02/26