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

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(-)

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>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

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.

> +
> +
> +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.
> +
> +    :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.
> +    """
> +    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
> +    if match and match.end() != len(prefix):

Nice catch with the extra check here.  Maybe worth mentioning and/or
splitting the change?

> +        msg = "funny character '{:s}' in prefix '{:s}'".format(
> +            prefix[match.end()], prefix)
> +        raise QAPIError('', None, msg)
> +
> +    schema = QAPISchema(schema_file)
> +    gen_types(schema, output_dir, prefix, builtins)
> +    gen_visit(schema, output_dir, prefix, builtins)
> +    gen_commands(schema, output_dir, prefix)
> +    gen_events(schema, output_dir, prefix)
> +    gen_introspect(schema, output_dir, prefix, unmask)
> +    gen_doc(schema, output_dir, prefix)
> +
> +
> +def main() -> int:

One extra Pythonic touch would be to use a bool here, and then:

  sys.exit(0 if main() else 1)

But that's probably overkill.

> +    """
> +    gapi-gen shell script entrypoint.
> +    Expects arguments via sys.argv, see --help for details.
> +
> +    :return: int, 0 on success, 1 on failure.
> +    """
>      parser = argparse.ArgumentParser(
>          description='Generate code from a QAPI schema')
>      parser.add_argument('-b', '--builtins', action='store_true',
>                          help="generate code for built-in types")
> -    parser.add_argument('-o', '--output-dir', action='store', default='',
> +    parser.add_argument('-o', '--output-dir', action='store',
> +                        default=DEFAULT_OUTPUT_DIR,
>                          help="write output to directory OUTPUT_DIR")
> -    parser.add_argument('-p', '--prefix', action='store', default='',
> +    parser.add_argument('-p', '--prefix', action='store',
> +                        default=DEFAULT_PREFIX,
>                          help="prefix for symbols")
>      parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>                          dest='unmask',
> @@ -33,26 +81,17 @@ def main(argv):
>      parser.add_argument('schema', action='store')
>      args = parser.parse_args()
>  
> -    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', args.prefix)
> -    if match.end() != len(args.prefix):
> -        print("%s: 'funny character '%s' in argument of --prefix"
> -              % (sys.argv[0], args.prefix[match.end()]),
> -              file=sys.stderr)
> -        sys.exit(1)
> -
>      try:
> -        schema = QAPISchema(args.schema)
> +        generate(args.schema,
> +                 output_dir=args.output_dir,
> +                 prefix=args.prefix,
> +                 unmask=args.unmask,
> +                 builtins=args.builtins)
>      except QAPIError as err:
> -        print(err, file=sys.stderr)
> -        exit(1)
> -

Glad to see that this "quitter" is gone in favor of one and only
sys.exit().

- Cleber.

> -    gen_types(schema, args.output_dir, args.prefix, args.builtins)
> -    gen_visit(schema, args.output_dir, args.prefix, args.builtins)
> -    gen_commands(schema, args.output_dir, args.prefix)
> -    gen_events(schema, args.output_dir, args.prefix)
> -    gen_introspect(schema, args.output_dir, args.prefix, args.unmask)
> -    gen_doc(schema, args.output_dir, args.prefix)
> +        print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
> +        return 1
> +    return 0
>  
>  
>  if __name__ == '__main__':
> -    main(sys.argv)
> +    sys.exit(main())
> -- 
> 2.26.2
>

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.

You found them! My plan worked.

>> +
>> +
>> +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.
>> +
>> +    :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.
>> +    """
>> +    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
>> +    if match and match.end() != len(prefix):
> 
> Nice catch with the extra check here.  Maybe worth mentioning and/or
> splitting the change?
> 

If you review all 125 patches, I will do this for you ;)

>> +        msg = "funny character '{:s}' in prefix '{:s}'".format(
>> +            prefix[match.end()], prefix)
>> +        raise QAPIError('', None, msg)
>> +
>> +    schema = QAPISchema(schema_file)
>> +    gen_types(schema, output_dir, prefix, builtins)
>> +    gen_visit(schema, output_dir, prefix, builtins)
>> +    gen_commands(schema, output_dir, prefix)
>> +    gen_events(schema, output_dir, prefix)
>> +    gen_introspect(schema, output_dir, prefix, unmask)
>> +    gen_doc(schema, output_dir, prefix)
>> +
>> +
>> +def main() -> int:
> 
> One extra Pythonic touch would be to use a bool here, and then:
> 
>    sys.exit(0 if main() else 1)
> 
> But that's probably overkill.
> 

I think a function named main() is fair enough to return int -- we are 
declaring that this is a shell script pretty explicitly, and it is 
allowed to return a status code.

Shifting the knowledge of how shell codes work up one more layer is 
probably not ... urgent.

>> +    """
>> +    gapi-gen shell script entrypoint.
>> +    Expects arguments via sys.argv, see --help for details.
>> +
>> +    :return: int, 0 on success, 1 on failure.
>> +    """
>>       parser = argparse.ArgumentParser(
>>           description='Generate code from a QAPI schema')
>>       parser.add_argument('-b', '--builtins', action='store_true',
>>                           help="generate code for built-in types")
>> -    parser.add_argument('-o', '--output-dir', action='store', default='',
>> +    parser.add_argument('-o', '--output-dir', action='store',
>> +                        default=DEFAULT_OUTPUT_DIR,
>>                           help="write output to directory OUTPUT_DIR")
>> -    parser.add_argument('-p', '--prefix', action='store', default='',
>> +    parser.add_argument('-p', '--prefix', action='store',
>> +                        default=DEFAULT_PREFIX,
>>                           help="prefix for symbols")
>>       parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>>                           dest='unmask',
>> @@ -33,26 +81,17 @@ def main(argv):
>>       parser.add_argument('schema', action='store')
>>       args = parser.parse_args()
>>   
>> -    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', args.prefix)
>> -    if match.end() != len(args.prefix):
>> -        print("%s: 'funny character '%s' in argument of --prefix"
>> -              % (sys.argv[0], args.prefix[match.end()]),
>> -              file=sys.stderr)
>> -        sys.exit(1)
>> -
>>       try:
>> -        schema = QAPISchema(args.schema)
>> +        generate(args.schema,
>> +                 output_dir=args.output_dir,
>> +                 prefix=args.prefix,
>> +                 unmask=args.unmask,
>> +                 builtins=args.builtins)
>>       except QAPIError as err:
>> -        print(err, file=sys.stderr)
>> -        exit(1)
>> -
> 
> Glad to see that this "quitter" is gone in favor of one and only
> sys.exit().
> 
> - Cleber.
> 
>> -    gen_types(schema, args.output_dir, args.prefix, args.builtins)
>> -    gen_visit(schema, args.output_dir, args.prefix, args.builtins)
>> -    gen_commands(schema, args.output_dir, args.prefix)
>> -    gen_events(schema, args.output_dir, args.prefix)
>> -    gen_introspect(schema, args.output_dir, args.prefix, args.unmask)
>> -    gen_doc(schema, args.output_dir, args.prefix)
>> +        print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
>> +        return 1
>> +    return 0
>>   
>>   
>>   if __name__ == '__main__':
>> -    main(sys.argv)
>> +    sys.exit(main())
>> -- 
>> 2.26.2
>>

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.

Cleber Rosa <crosa@redhat.com> writes:

> 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.
>
>> +
>> +
>> +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.
>> +
>> +    :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.
>> +    """
>> +    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
>> +    if match and match.end() != len(prefix):
>
> Nice catch with the extra check here.  Maybe worth mentioning and/or
> splitting the change?

Please do not sneak additional checking into patches advertized as pure
refactoring.  It makes me look for more sneakery with a microscope.

This re.match() cannot possibly fail.  Three cases:

* First character is funny

  The regexp matches the empty string.  There's a reason the regexp ends
  with '?'.

* Non-first character is funny

  The regexp matches the non-funny prefix.

* No character is funny

  The regexp matches the complete string.

Checking impossible conditions as if they were possible is confusing.
Please drop the additional check.

We can talk about checking this impossible condition with

        assert(match)

if you believe it makes the code easier to understand (it does not
improve its behavior).

>> +        msg = "funny character '{:s}' in prefix '{:s}'".format(
>> +            prefix[match.end()], prefix)
>> +        raise QAPIError('', None, msg)
>> +
>> +    schema = QAPISchema(schema_file)
>> +    gen_types(schema, output_dir, prefix, builtins)
>> +    gen_visit(schema, output_dir, prefix, builtins)
>> +    gen_commands(schema, output_dir, prefix)
>> +    gen_events(schema, output_dir, prefix)
>> +    gen_introspect(schema, output_dir, prefix, unmask)
>> +    gen_doc(schema, output_dir, prefix)
>> +
>> +
>> +def main() -> int:
>
> One extra Pythonic touch would be to use a bool here, and then:
>
>   sys.exit(0 if main() else 1)
>
> But that's probably overkill.

Yuck :)

>> +    """
>> +    gapi-gen shell script entrypoint.
>> +    Expects arguments via sys.argv, see --help for details.
>> +
>> +    :return: int, 0 on success, 1 on failure.
>> +    """
>>      parser = argparse.ArgumentParser(
>>          description='Generate code from a QAPI schema')
>>      parser.add_argument('-b', '--builtins', action='store_true',
>>                          help="generate code for built-in types")
>> -    parser.add_argument('-o', '--output-dir', action='store', default='',
>> +    parser.add_argument('-o', '--output-dir', action='store',
>> +                        default=DEFAULT_OUTPUT_DIR,
>>                          help="write output to directory OUTPUT_DIR")
>> -    parser.add_argument('-p', '--prefix', action='store', default='',
>> +    parser.add_argument('-p', '--prefix', action='store',
>> +                        default=DEFAULT_PREFIX,
>>                          help="prefix for symbols")
>>      parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
>>                          dest='unmask',
>> @@ -33,26 +81,17 @@ def main(argv):
>>      parser.add_argument('schema', action='store')
>>      args = parser.parse_args()
>>  
>> -    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', args.prefix)
>> -    if match.end() != len(args.prefix):
>> -        print("%s: 'funny character '%s' in argument of --prefix"
>> -              % (sys.argv[0], args.prefix[match.end()]),
>> -              file=sys.stderr)
>> -        sys.exit(1)
>> -
>>      try:
>> -        schema = QAPISchema(args.schema)
>> +        generate(args.schema,
>> +                 output_dir=args.output_dir,
>> +                 prefix=args.prefix,
>> +                 unmask=args.unmask,
>> +                 builtins=args.builtins)
>>      except QAPIError as err:
>> -        print(err, file=sys.stderr)
>> -        exit(1)
>> -
>
> Glad to see that this "quitter" is gone in favor of one and only
> sys.exit().

I'm totally fine with having multiple sys.exit() in main().  They become
icky only when buried too deeply.

I'm fine with John's patch regardless.

[...]

On 9/25/20 7:34 AM, Markus Armbruster wrote:
> Cleber Rosa <crosa@redhat.com> writes:
> 
>> 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.
>>
>>> +
>>> +
>>> +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.
>>> +
>>> +    :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.
>>> +    """
>>> +    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
>>> +    if match and match.end() != len(prefix):
>>
>> Nice catch with the extra check here.  Maybe worth mentioning and/or
>> splitting the change?
> 
> Please do not sneak additional checking into patches advertized as pure
> refactoring.  It makes me look for more sneakery with a microscope.
> 
> This re.match() cannot possibly fail.  Three cases:
> 
> * First character is funny
> 
>    The regexp matches the empty string.  There's a reason the regexp ends
>    with '?'.
> 
> * Non-first character is funny
> 
>    The regexp matches the non-funny prefix.
> 
> * No character is funny
> 
>    The regexp matches the complete string.
> 
> Checking impossible conditions as if they were possible is confusing.
> Please drop the additional check.
> 
> We can talk about checking this impossible condition with
> 
>          assert(match)
> 
> if you believe it makes the code easier to understand (it does not
> improve its behavior).
> 

My use of strict_optional=False is what prevents this from exhibiting as 
an error in mypy. An assert will help convince mypy that 'match' cannot 
possibly be 'None'.

eh, well. I will fix this when I remove strict_optional, so I will just 
remove this additional check for now to avoid adding another patch to 
this series.

--js

John Snow <jsnow@redhat.com> writes:

> On 9/25/20 7:34 AM, Markus Armbruster wrote:
>> Cleber Rosa <crosa@redhat.com> writes:
>> 
>>> 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>
[...]
>>>> +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.
>>>> +
>>>> +    :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.
>>>> +    """
>>>> +    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
>>>> +    if match and match.end() != len(prefix):
>>>
>>> Nice catch with the extra check here.  Maybe worth mentioning and/or
>>> splitting the change?
>>
>> Please do not sneak additional checking into patches advertized as pure
>> refactoring.  It makes me look for more sneakery with a microscope.
>> 
>> This re.match() cannot possibly fail.  Three cases:
>> 
>> * First character is funny
>> 
>>   The regexp matches the empty string.  There's a reason the regexp ends
>>   with '?'.
>> 
>> * Non-first character is funny
>> 
>>   The regexp matches the non-funny prefix.
>> 
>> * No character is funny
>> 
>>   The regexp matches the complete string.
>> 
>> Checking impossible conditions as if they were possible is confusing.
>> Please drop the additional check.
>> 
>> We can talk about checking this impossible condition with
>> 
>>         assert(match)
>> 
>> if you believe it makes the code easier to understand (it does not
>> improve its behavior).
>
> My use of strict_optional=False is what prevents this from exhibiting
> as an error in mypy. An assert will help convince mypy that 'match'
> cannot possibly be 'None'.

Adding assertions to help mypy along is okay.

> eh, well. I will fix this when I remove strict_optional, so I will
> just remove this additional check for now to avoid adding another
> patch to this series.

Makes sense to me.