| Message ID | 20231016152704.221611-3-victortoso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi-go: add generator for Golang interface | expand |
Victor Toso <victortoso@redhat.com> writes: > flake8 complained: > ./main.py:60:1: E302 expected 2 blank lines, found 1 > > Which is simple enough. My vim has black [0] enabled by default, so it > did some extra formatting. I'm proposing to follow it. > > [0] https://black.readthedocs.io/en/stable/ > > Signed-off-by: Victor Toso <victortoso@redhat.com> > --- > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++---------------- > 1 file changed, 48 insertions(+), 28 deletions(-) Is this all black hates about scripts/qapi/? Did you configure it in any way, and if yes, how? [...]
On Wed, Oct 18, 2023 at 01:00:07PM +0200, Markus Armbruster wrote: > Victor Toso <victortoso@redhat.com> writes: > > > flake8 complained: > > ./main.py:60:1: E302 expected 2 blank lines, found 1 > > > > Which is simple enough. My vim has black [0] enabled by default, so it > > did some extra formatting. I'm proposing to follow it. > > > > [0] https://black.readthedocs.io/en/stable/ > > > > Signed-off-by: Victor Toso <victortoso@redhat.com> > > --- > > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++---------------- > > 1 file changed, 48 insertions(+), 28 deletions(-) > > Is this all black hates about scripts/qapi/? > > Did you configure it in any way, and if yes, how? The point of the 'black' tool is that it be highly opinionated and bring (force) all projects in the python code into following the same style. As such it intentionally has near zero configuration options. You can control the line length it uses (88 by default) and something related to string quoting style normalization, but that's basically it. Generally though developers should just run 'black' and accept all the changes it makes without questioning. Personally I'd encourage the use of black for any project with python code, precisely to remove any need to spend time debating code style. If a project is also using flake8 there's a config needed for flake8 to stop it conflicting with black though eg in $gitroot/.flake8 you'd need at least: [flake8] max-line-length = 88 extend-ignore = E203 If QEMU intends to adopt black, at the very least we need to have it validated by CI and probably by 'make check' too, to avoid regressions. With regards, Daniel
On Wed, Oct 18, 2023 at 01:00:07PM +0200, Markus Armbruster wrote: > Victor Toso <victortoso@redhat.com> writes: > > > flake8 complained: > > ./main.py:60:1: E302 expected 2 blank lines, found 1 > > > > Which is simple enough. My vim has black [0] enabled by default, so it > > did some extra formatting. I'm proposing to follow it. > > > > [0] https://black.readthedocs.io/en/stable/ > > > > Signed-off-by: Victor Toso <victortoso@redhat.com> > > --- > > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++---------------- > > 1 file changed, 48 insertions(+), 28 deletions(-) > > Is this all black hates about scripts/qapi/? No, just scripts/qapi/main.py. > Did you configure it in any way, and if yes, how? Only to reduce line length to 79. I can do a separate series for this, if the idea is accepted. Cheers, Victor
Victor Toso <victortoso@redhat.com> writes: > On Wed, Oct 18, 2023 at 01:00:07PM +0200, Markus Armbruster wrote: >> Victor Toso <victortoso@redhat.com> writes: >> >> > flake8 complained: >> > ./main.py:60:1: E302 expected 2 blank lines, found 1 >> > >> > Which is simple enough. My vim has black [0] enabled by default, so it >> > did some extra formatting. I'm proposing to follow it. >> > >> > [0] https://black.readthedocs.io/en/stable/ >> > >> > Signed-off-by: Victor Toso <victortoso@redhat.com> >> > --- >> > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++---------------- >> > 1 file changed, 48 insertions(+), 28 deletions(-) >> >> Is this all black hates about scripts/qapi/? > > No, just scripts/qapi/main.py. > >> Did you configure it in any way, and if yes, how? > > Only to reduce line length to 79. > > I can do a separate series for this, if the idea is accepted. Let's build a rough idea of how much churn this would be. We have a bit over 5000 lines: $ wc -l scripts/qapi/*py 419 scripts/qapi/commands.py 251 scripts/qapi/common.py 50 scripts/qapi/error.py 251 scripts/qapi/events.py 679 scripts/qapi/expr.py 368 scripts/qapi/gen.py 390 scripts/qapi/introspect.py 103 scripts/qapi/main.py 777 scripts/qapi/parser.py 1233 scripts/qapi/schema.py 71 scripts/qapi/source.py 387 scripts/qapi/types.py 429 scripts/qapi/visit.py 5408 total Feed them to black: $ black -q -l 75 scripts/qapi $ git-diff --stat scripts/qapi/commands.py | 448 +++++++++++++++++----------- scripts/qapi/common.py | 240 ++++++++++----- scripts/qapi/error.py | 15 +- scripts/qapi/events.py | 274 +++++++++++------- scripts/qapi/expr.py | 409 ++++++++++++++++---------- scripts/qapi/gen.py | 187 +++++++----- scripts/qapi/introspect.py | 323 +++++++++++++-------- scripts/qapi/main.py | 80 +++-- scripts/qapi/parser.py | 370 +++++++++++++---------- scripts/qapi/schema.py | 709 +++++++++++++++++++++++++++++---------------- scripts/qapi/source.py | 17 +- scripts/qapi/types.py | 369 ++++++++++++++--------- scripts/qapi/visit.py | 355 ++++++++++++++--------- 13 files changed, 2383 insertions(+), 1413 deletions(-) *Ouch* Peeking at the result, I see string quote normalization. Try again with that switched off, and the line length relaxed: $ black -q -l 79 -S scripts/qapi $ git-diff --stat scripts/qapi/commands.py | 357 +++++++++++++++++++++------------ scripts/qapi/common.py | 170 ++++++++++++---- scripts/qapi/error.py | 11 +- scripts/qapi/events.py | 220 +++++++++++++------- scripts/qapi/expr.py | 261 +++++++++++++++--------- scripts/qapi/gen.py | 114 ++++++----- scripts/qapi/introspect.py | 231 +++++++++++++-------- scripts/qapi/main.py | 72 ++++--- scripts/qapi/parser.py | 224 ++++++++++++--------- scripts/qapi/schema.py | 488 +++++++++++++++++++++++++++++++-------------- scripts/qapi/source.py | 7 +- scripts/qapi/types.py | 303 ++++++++++++++++++---------- scripts/qapi/visit.py | 287 ++++++++++++++++---------- 13 files changed, 1802 insertions(+), 943 deletions(-) Still massive churn.
On Thu, Oct 19, 2023 at 07:42:38AM +0200, Markus Armbruster wrote: > Victor Toso <victortoso@redhat.com> writes: > > > On Wed, Oct 18, 2023 at 01:00:07PM +0200, Markus Armbruster wrote: > >> Victor Toso <victortoso@redhat.com> writes: > >> > >> > flake8 complained: > >> > ./main.py:60:1: E302 expected 2 blank lines, found 1 > >> > > >> > Which is simple enough. My vim has black [0] enabled by default, so it > >> > did some extra formatting. I'm proposing to follow it. > >> > > >> > [0] https://black.readthedocs.io/en/stable/ > >> > > >> > Signed-off-by: Victor Toso <victortoso@redhat.com> > >> > --- > >> > scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++---------------- > >> > 1 file changed, 48 insertions(+), 28 deletions(-) > >> > >> Is this all black hates about scripts/qapi/? > > > > No, just scripts/qapi/main.py. > > > >> Did you configure it in any way, and if yes, how? > > > > Only to reduce line length to 79. > > > > I can do a separate series for this, if the idea is accepted. > > Let's build a rough idea of how much churn this would be. > > We have a bit over 5000 lines: > > $ wc -l scripts/qapi/*py > 419 scripts/qapi/commands.py > 251 scripts/qapi/common.py > 50 scripts/qapi/error.py > 251 scripts/qapi/events.py > 679 scripts/qapi/expr.py > 368 scripts/qapi/gen.py > 390 scripts/qapi/introspect.py > 103 scripts/qapi/main.py > 777 scripts/qapi/parser.py > 1233 scripts/qapi/schema.py > 71 scripts/qapi/source.py > 387 scripts/qapi/types.py > 429 scripts/qapi/visit.py > 5408 total > > Feed them to black: > > $ black -q -l 75 scripts/qapi > $ git-diff --stat > scripts/qapi/commands.py | 448 +++++++++++++++++----------- > scripts/qapi/common.py | 240 ++++++++++----- > scripts/qapi/error.py | 15 +- > scripts/qapi/events.py | 274 +++++++++++------- > scripts/qapi/expr.py | 409 ++++++++++++++++---------- > scripts/qapi/gen.py | 187 +++++++----- > scripts/qapi/introspect.py | 323 +++++++++++++-------- > scripts/qapi/main.py | 80 +++-- > scripts/qapi/parser.py | 370 +++++++++++++---------- > scripts/qapi/schema.py | 709 +++++++++++++++++++++++++++++---------------- > scripts/qapi/source.py | 17 +- > scripts/qapi/types.py | 369 ++++++++++++++--------- > scripts/qapi/visit.py | 355 ++++++++++++++--------- > 13 files changed, 2383 insertions(+), 1413 deletions(-) > > *Ouch* > > Peeking at the result, I see string quote normalization. Try again with > that switched off, and the line length relaxed: > > $ black -q -l 79 -S scripts/qapi > $ git-diff --stat > scripts/qapi/commands.py | 357 +++++++++++++++++++++------------ > scripts/qapi/common.py | 170 ++++++++++++---- > scripts/qapi/error.py | 11 +- > scripts/qapi/events.py | 220 +++++++++++++------- > scripts/qapi/expr.py | 261 +++++++++++++++--------- > scripts/qapi/gen.py | 114 ++++++----- > scripts/qapi/introspect.py | 231 +++++++++++++-------- > scripts/qapi/main.py | 72 ++++--- > scripts/qapi/parser.py | 224 ++++++++++++--------- > scripts/qapi/schema.py | 488 +++++++++++++++++++++++++++++++-------------- > scripts/qapi/source.py | 7 +- > scripts/qapi/types.py | 303 ++++++++++++++++++---------- > scripts/qapi/visit.py | 287 ++++++++++++++++---------- > 13 files changed, 1802 insertions(+), 943 deletions(-) > > Still massive churn. FWIW, the .git-blame-ignore-revs file can be populated with commit hashes afterwards, to make 'git blame' ignore the reformatting changes. Doesn't help with people cherry-picking fixes to old branches though, which is the other main downside of such churn. With regards, Daniel
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py index 316736b6a2..fe65c1a17a 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -22,18 +22,20 @@ def invalid_prefix_char(prefix: str) -> Optional[str]: - match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix) + match = must_match(r"([A-Za-z_.-][A-Za-z0-9_.-]*)?", prefix) if match.end() != len(prefix): return prefix[match.end()] return None -def generate(schema_file: str, - output_dir: str, - prefix: str, - unmask: bool = False, - builtins: bool = False, - gen_tracing: bool = False) -> None: +def generate( + schema_file: str, + output_dir: str, + prefix: str, + unmask: bool = False, + builtins: bool = False, + gen_tracing: bool = False, +) -> None: """ Generate C code for the given schema into the target directory. @@ -63,25 +65,41 @@ def main() -> int: :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='', - help="write output to directory OUTPUT_DIR") - parser.add_argument('-p', '--prefix', action='store', - default='', - help="prefix for symbols") - parser.add_argument('-u', '--unmask-non-abi-names', action='store_true', - dest='unmask', - help="expose non-ABI names in introspection") + 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="", + help="write output to directory OUTPUT_DIR", + ) + parser.add_argument( + "-p", "--prefix", action="store", default="", help="prefix for symbols" + ) + parser.add_argument( + "-u", + "--unmask-non-abi-names", + action="store_true", + dest="unmask", + help="expose non-ABI names in introspection", + ) # Option --suppress-tracing exists so we can avoid solving build system # problems. TODO Drop it when we no longer need it. - parser.add_argument('--suppress-tracing', action='store_true', - help="suppress adding trace events to qmp marshals") + parser.add_argument( + "--suppress-tracing", + action="store_true", + help="suppress adding trace events to qmp marshals", + ) - parser.add_argument('schema', action='store') + parser.add_argument("schema", action="store") args = parser.parse_args() funny_char = invalid_prefix_char(args.prefix) @@ -91,12 +109,14 @@ def main() -> int: return 1 try: - generate(args.schema, - output_dir=args.output_dir, - prefix=args.prefix, - unmask=args.unmask, - builtins=args.builtins, - gen_tracing=not args.suppress_tracing) + generate( + args.schema, + output_dir=args.output_dir, + prefix=args.prefix, + unmask=args.unmask, + builtins=args.builtins, + gen_tracing=not args.suppress_tracing, + ) except QAPIError as err: print(err, file=sys.stderr) return 1
flake8 complained: ./main.py:60:1: E302 expected 2 blank lines, found 1 Which is simple enough. My vim has black [0] enabled by default, so it did some extra formatting. I'm proposing to follow it. [0] https://black.readthedocs.io/en/stable/ Signed-off-by: Victor Toso <victortoso@redhat.com> --- scripts/qapi/main.py | 76 ++++++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 28 deletions(-)