Re: [PATCH v2 02/38] qapi-gen: Separate arg-parsing from generation

[Cleber Rosa]

On Wed, Sep 23, 2020 at 01:05:47PM -0400, John Snow wrote:
> On 9/22/20 8:00 PM, Cleber Rosa wrote:
> > On Tue, Sep 22, 2020 at 05:00:25PM -0400, John Snow wrote:
> > > This is a minor re-work of the entrypoint script. It isolates a
> > > generate() method from the actual command-line mechanism.
> > > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   scripts/qapi-gen.py | 87 ++++++++++++++++++++++++++++++++-------------
> > >   1 file changed, 63 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> > > index 4b03f7d53b..59becba3e1 100644
> > > --- a/scripts/qapi-gen.py
> > > +++ b/scripts/qapi-gen.py
> > > @@ -1,9 +1,13 @@
> > >   #!/usr/bin/env python3
> > > -# QAPI generator
> > > -#
> > > +
> > >   # This work is licensed under the terms of the GNU GPL, version 2 or 
> > > later.
> > >   # See the COPYING file in the top-level directory.
> > > +"""
> > > +QAPI Generator
> > > +
> > > +This script is the main entry point for generating C code from the QAPI 
> > > schema.
> > > +"""
> > >   import argparse
> > >   import re
> > > @@ -11,21 +15,65 @@
> > >   from qapi.commands import gen_commands
> > >   from qapi.doc import gen_doc
> > > +from qapi.error import QAPIError
> > >   from qapi.events import gen_events
> > >   from qapi.introspect import gen_introspect
> > > -from qapi.schema import QAPIError, QAPISchema
> > > +from qapi.schema import QAPISchema
> > >   from qapi.types import gen_types
> > >   from qapi.visit import gen_visit
> > > -def main(argv):
> > > +DEFAULT_OUTPUT_DIR = ''
> > > +DEFAULT_PREFIX = ''
> > 
> > I did not understand the purpose of these.  If they're used only as
> > the default value for the command line option parsing, I'd suggest
> > dropping them.
> > 
> 
> The alternative is setting default='' inline below, which is fine, but found
> them kind of buried; and looking a bit too much like a weird magic constant.
> Aesthetically, I liked making them obvious.
>

I differ in opinion and style, and I don't think you should adopt
mine.  Just for the record, though, consider if would do the same for
dozens of command line options... IMO it'd be fine to declare them for
extra explicitness, but I see no reason for them to be module globals.

> You found them! My plan worked.
>

:)

- Cleber.

signature.asc
Description: PGP signature