|
| From: | John Snow |
| Subject: | Re: [PATCH v2 02/38] qapi-gen: Separate arg-parsing from generation |
| Date: | Wed, 23 Sep 2020 13:05:47 -0400 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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 argparseimport re @@ -11,21 +15,65 @@from qapi.commands import gen_commandsfrom 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 0if __name__ == '__main__':- main(sys.argv) + sys.exit(main()) -- 2.26.2
| [Prev in Thread] | Current Thread | [Next in Thread] |