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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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?")

Emacs Python mode is close enough by default: fill-paragraph (bound to
M-q) uses variable fill-column, which defaults to 70.  If you want the
extra two columns PEP 8 grants you, I can show you how to bump it to 72
just for Python mode.

You can use fill-paragraph for code, too.  I don't myself, because I
disagree with its line breaking decisions too often (and so does PEP 8).
A better Python mode would break code lines more neatly, and with the
width defaulting to 79.

>>> +"""
>>>     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.

Me rewriting your commits without your consent is putting words in your
mouth, which I don't want to do.

We can still reduce ping-pong: whenever I can, I don't just say "this
needs improvement", I propose improvements.  If you disagree, we talk.
Else, if you have to respin, you make a reasonable effort to take them.
Else, the remaining improvements are trivial (because no respin), and
I'll make them in my tree.

>>> +
>>> +    :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.
>>> +    """
>> [...]
>>