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

[Collin Funk]

Hi Bruno,

Hopefully this patch should be better. Feel free to let me know if
there is anything I didn't address.

On 3/10/24 5:07 AM, Bruno Haible wrote:
> 1) This form of conditional expression
> 
>        base = self.config['destdir'] if self.config['destdir'] else '.'
> 
>    mentions first a value, then a condition, then another value.
>    It would be good to mention the condition first, like in C, Lisp, Java, 
> etc.
>    - even if that means that this needs 4 lines of code instead of 1 line.

Sure, thanks for updating the coding style. I've changed it to a more
normal:

        if self.config['destdir']:
            base = self.config['destdir']
        else:
            base = '.'

>    automake_options = { x
>                         for y in automake_options
>                         for x in y.split() }
> 
>    It is more readable this way.

Oops, I forgot about this convention. Fixed.

> 3) Method GLConfig.setAutomakeSubdir claims that
>    "automake_subdir must be a string"
>    but it should be bool.

I think I see what you mean. I updated the type check but forgot to
update the comment and message below it. Should be fixed now.

> 4) In GLEmiter.initmacro_end, you translated the condition
> 
>    if $automake_subdir && ! "$2" && test -n "$sourcebase" && test 
> "$sourcebase" != '.'; then
> 
>    to
> 
>    if automake_subdir and not gentests and sourcebase != '.':
> 
>    What about the case sourcebase == '' ? We don't want subdir to be '/' in 
> this case.
>    (Or can this not happen?)

Oops, for some reason my brain must have ignored the
'test -n "$sourcebase"'. I am not 100% sure of all the paths that
sourcebase can enter this function with. In any case, I think it is
best to add the check. After a call to GLConfig.resetSourceBase() it
may be '', and that is probably likely to occur.

I'm not the best with shell and always have to look up the different
test flags. I believe that this condition should work according to my
interpretation of 'test -n' [1]:

    if automake_subdir and not gentests and sourcebase and sourcebase != '.':

In this case the condition will fail if sourcebase is a string with a
length of zero. Comparing to '' feels strange to me, but let me know if
you prefer it.

> 5) GLEmiter.shellvars_init has no function comment. How about
> 
>    def shellvars_init(self, gentests: bool, base: str) -> str:
>        '''emits some shell variable assignments'''

I've added this along with a description of each parameter, mostly
copied from the regular gnulib-tool.

>    Also please don't omit the comments that describe what gl_source_base and
>    gl_source_base_prefix. Because these are really hard to grasp without a
>    comment.

I agree. I assume that you mean the ones that are written in the shell
script? I forgot to copy and paste those. I am still learning how
gnulib-tool works so it is a bit hard for me to add original comments
for these, sorry. For example, I just learned how --local-dir worked
with Coreutils usage of it. That option seems very useful and I'm a
bit surprised I just noticed it...

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html

Collin

0001-gnulib-tool.py-Follow-gnulib-tool-changes-part-51.patch
Description: Text Data