Re: [PATCH v5 03/36] qapi-gen: Separate arg-parsing from generation

[John Snow]

On 10/7/20 4:12 AM, Markus Armbruster wrote:
> I keep stumbling over things in later patches that turn out to go back
> to this one.
>
> John Snow <jsnow@redhat.com> writes:
>
> > 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>
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > Tested-by: Cleber Rosa <crosa@redhat.com>
> > ---
> >   scripts/qapi-gen.py | 85 +++++++++++++++++++++++++++++++++------------
> >   1 file changed, 62 insertions(+), 23 deletions(-)
> >
> > diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> > index 541e8c1f55d..117b396a595 100644
> > --- a/scripts/qapi-gen.py
> > +++ b/scripts/qapi-gen.py
> > @@ -1,30 +1,77 @@
> >   #!/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.
>
> PEP 8: For flowing long blocks of text with fewer structural
> restrictions (docstrings or comments), the line length should be limited
> to 72 characters.

Eugh. OK, but I don't have a good way to check or enforce this, 
admittedly. I have to change my emacs settings to understand this when I 
hit the reflow key. I don't know if the python mode has a context-aware 
reflow length.

("I don't disagree, but I'm not immediately sure right now how I will 
make sure I, or anyone else, complies with this. Low priority as a result?")

> > +"""
> >   
> >   import argparse
> >   import re
> >   import sys
> >   
> >   from qapi.commands import gen_commands
> > +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 = ''
> > +
> > +
> > +def generate(schema_file: str,
> > +             output_dir: str,
> > +             prefix: str,
> > +             unmask: bool = False,
> > +             builtins: bool = False) -> None:
> > +    """
> > +    generate uses a given schema to produce C code in the target directory.
>
> PEP 257: The docstring is a phrase ending in a period.  It prescribes
> the function or method's effect as a command ("Do this", "Return that"),
> not as a description; e.g. don't write "Returns the pathname ...".
>
> Suggest
>
>         Generate C code for the given schema into the target directory.

OK. I don't mind trying to foster a consistent tone. I clearly didn't. I 
will add a note to my style guide todo.

I give you permission to change the voice in any of my docstrings, or to 
adjust the phrasing to be more technically accurate as you see fit. You 
are the primary maintainer of this code, of course, and you will know best.

It will be quicker to give you full permission to just change any of the 
docstrings as you see fit than it will be to play review-respin ping-pong.

> > +
> > +    :param schema_file: The primary QAPI schema file.
> > +    :param output_dir: The output directory to store generated code.
> > +    :param prefix: Optional C-code prefix for symbol names.
> > +    :param unmask: Expose non-ABI names through introspection?
> > +    :param builtins: Generate code for built-in types?
> > +
> > +    :raise QAPIError: On failures.
> > +    """
> [...]