Re: [PATCH v3 12/47] qapi: enforce import order/styling with isort

[John Snow]

On 9/25/20 5:20 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > While we're mucking around with imports, we might as well formalize the
> > style we use. Let's use isort to do it for us.
> >
> > force_sort_within_sections: Intermingles "from x" and "import x" style
> > statements, such that sorting is always performed strictly on the module
> > name itself.
> >
> > force_grid_wrap=4: Four or more imports from a single module will force
> > the one-per-line style that's more git-friendly. This will generally
> > happen for 'typing' imports.
> >
> > multi_line_output=3: Uses the one-per-line indented style for long
> > imports.
> >
> > include_trailing_comma: Adds a comma to the last import in a group,
> > which makes git conflicts nicer to deal with, generally.
> >
> > Suggested-by: Cleber Rosa <crosa@redhat.com>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/.isort.cfg    | 5 +++++
> >   scripts/qapi/commands.py   | 6 +-----
> >   scripts/qapi/doc.py        | 2 +-
> >   scripts/qapi/expr.py       | 4 ++--
> >   scripts/qapi/introspect.py | 3 +--
> >   scripts/qapi/main.py       | 1 -
> >   scripts/qapi/parser.py     | 2 +-
> >   scripts/qapi/schema.py     | 2 +-
> >   scripts/qapi/types.py      | 1 -
> >   9 files changed, 12 insertions(+), 14 deletions(-)
> >   create mode 100644 scripts/qapi/.isort.cfg
> >
> > diff --git a/scripts/qapi/.isort.cfg b/scripts/qapi/.isort.cfg
> > new file mode 100644
> > index 0000000000..b0aeffec26
> > --- /dev/null
> > +++ b/scripts/qapi/.isort.cfg
> > @@ -0,0 +1,5 @@
> > +[settings]
> > +force_sort_within_sections=True
> > +force_grid_wrap=4
> > +multi_line_output=3
> > +include_trailing_comma=True
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index e1df0e341f..64ed5278f9 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -13,11 +13,7 @@
> >   See the COPYING file in the top-level directory.
> >   """
> >   
> > -from .common import (
> > -    build_params,
> > -    c_name,
> > -    mcgen,
> > -)
> > +from .common import build_params, c_name, mcgen
> >   from .gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
>
> Squash this hunk into the previous commit, please.

Yes, OK.

> >   
> >   
> > diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> > index b764a8ccc0..1acb773e0a 100644
> > --- a/scripts/qapi/doc.py
> > +++ b/scripts/qapi/doc.py
> > @@ -5,9 +5,9 @@
> >   """This script produces the documentation of a qapi schema in texinfo 
> > format"""
> >   
> >   import re
> > +
> >   from .gen import QAPIGenDoc, QAPISchemaVisitor
> >   
> > -
>
> Do you delete the blank line to keep isort happy?

isort offers to actually edit your files for you, it isn't just a 
checker, it actually rewrites.

By default, it leaves one space below imports. You can configure it to 
add two. I didn't really have a strong preference, so I left it at the 
default.

I'm going to leave it at the default for now, but I can change it if you 
have a preference.

> More of the same below.
>
> >   _MSG = '''
> >   @deftypefn {type} {{}} {name}
> >   
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 03b31ecfc1..3e952a1462 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -14,12 +14,12 @@
> >   # This work is licensed under the terms of the GNU GPL, version 2.
> >   # See the COPYING file in the top-level directory.
> >   
> > -import re
> >   from collections import OrderedDict
> > +import re
> > +
> >   from .common import c_name
> >   from .error import QAPISemError
> >   
> > -
> >   # Names must be letters, numbers, -, and _.  They must start with letter,
> >   # except for downstream extensions which must start with __RFQDN_.
> >   # Dots are only valid in the downstream extension prefix.
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index b036fcf9ce..2850121cbd 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -17,8 +17,7 @@
> >       mcgen,
> >   )
> >   from .gen import QAPISchemaMonolithicCVisitor
> > -from .schema import (QAPISchemaArrayType, QAPISchemaBuiltinType,
> > -                     QAPISchemaType)
> > +from .schema import QAPISchemaArrayType, QAPISchemaBuiltinType, QAPISchemaType
>
> Line is rather long for my taste.

78 chars and three names, so it didn't trigger the grid-wrap condition 
of isort.

If you have an arbitrarily smaller number you'd like to use, I can feed 
that to isort's config file. 72?

> So far, we didn't even try to have a consistent Python style in QEMU, so
> I've enforced a consistent style I like within QAPI.

I am now trying!

> Having a consistent Python style in QEMU would be worth letting go of
> personal preferences.  Do you intend to pursue it?

100%.

--js