Re: [PATCH] makedocbook: Fix false report of unhandled texinfo command

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]

On 04 Nov 2022 13:50, Jon Turney wrote:
> On 31/10/2022 12:28, Mike Frysinger wrote:
> > On 31 Oct 2022 09:59, Jon Turney wrote:
> >> +    if match:
> >> +        print("texinfo command '%s' remains in output" % match.group(), file=sys.stderr)
> > 
> > this is a little dangerous in general as match.group() could return a tuple,
> > and this would fail.  if you want to use %, you should force a tuple.
> >    print("..." % (match.group(),), ...)
> 
> I must be missing something here, because I'm not sure in what sense 
> 'could' is being used. I think match.group() is defined to be the same 
> as match.group(0), which always returns a single string. (I guess this 
> could be written explicitly).

you are correct that match.group() is match.group(0) which guarantees to
return a string.  when i wrote this, i didn't have access to the python
manual, so i checked against help() in the python repl.  the docs there
are much more terse and don't include the situations as to when a str or
a tuple would be returned.

> So the danger pointed out seems to be that if parts of the code were 
> changed, without making other necessary changes, it would be wrong. But 
> I'm not sure the suggested change just to avoid that future possibility 
> makes things better or clearer.

i've been bitten by refactors breaking % usage that lacks test coverage
(which, practically speaking, i think you could say most of the python
code here lacks unittests, especially in error code paths), so i have an
allergy to code that uses % without guaranteeing somehow that a tuple is
being passed like this code is doing now.

i grok that `% foo` vs `% (foo,)` looks a bit funky when you aren't used
to it, but i think this is more of a muscle memory thing -- this is the
only way to write a single element tuple.

i would have suggested using a f-string, but i don't know what versions
of python you want to support as f-strings require Python 3.6+.

of course if you really want to use % with a single element, i think it's
wrong, but i'm not going to make a continued stink about it :p.

> >> +        exit(1)
> > 
> > scripts should never use exit(), only sys.exit().  although i see the current
> > script gets this wrong in a lot of places.
> > 
> > also you can simplify this -- sys.exit accepts a string that it'll print to
> > stderr and then exit non-zero.
> >    sys.exit(".....")
> 
> Yes, this is one of the first things I wrote in python, and 
> unfortunately, it shows.

i wrote python like a C programmer for quite a long time,
and it took a while to reform my brain for python idioms
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]