Re: [PATCH v2 05/38] qapi: Remove wildcard includes

[Cleber Rosa]

On Wed, Sep 23, 2020 at 01:21:37PM -0400, John Snow wrote:
> On 9/23/20 9:27 AM, Cleber Rosa wrote:
> > On Tue, Sep 22, 2020 at 05:00:28PM -0400, John Snow wrote:
> > > Wildcard includes become hard to manage when refactoring and dealing
> > > with circular dependencies with strictly typed mypy.
> > > 
> > > flake8 also flags each one as a warning, as it is not smart enough to
> > > know which names exist in the imported file.
> > > 
> > > Remove them and include things explicitly by name instead.
> > > 
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   scripts/qapi/commands.py   |  6 +++++-
> > >   scripts/qapi/events.py     |  7 ++++++-
> > >   scripts/qapi/gen.py        | 12 +++++++++---
> > >   scripts/qapi/introspect.py |  7 ++++++-
> > >   scripts/qapi/types.py      |  8 +++++++-
> > >   scripts/qapi/visit.py      | 10 +++++++++-
> > >   6 files changed, 42 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > > index ce5926146a..e1df0e341f 100644
> > > --- a/scripts/qapi/commands.py
> > > +++ b/scripts/qapi/commands.py
> > > @@ -13,7 +13,11 @@
> > >   See the COPYING file in the top-level directory.
> > >   """
> > > -from .common import *
> > > +from .common import (
> > > +    build_params,
> > > +    c_name,
> > > +    mcgen,
> > > +)
> > >   from .gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
> > 
> > Is this import style being suggested or enforced by any tool?  I've
> > been using isort with very good results (both as a check tool, and as
> > an emacs extension).  For instance, the block about would look like:
> > 
> >     from .common import build_params, c_name, mcgen
> >     from .gen import QAPIGenCCode, QAPISchemaModularCVisitor, ifcontext
> > 
> 
> Not enforced by any tool, no. Just subjective preference for git-friendly
> import lines. They conflict on rebase a lot less.
> 
> I have been using emacs sort-lines to order the names in a group.
> 
> > > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > > index 0467272438..6b3afa14d7 100644
> > > --- a/scripts/qapi/events.py
> > > +++ b/scripts/qapi/events.py
> > > @@ -12,7 +12,12 @@
> > >   See the COPYING file in the top-level directory.
> > >   """
> > > -from .common import *
> > > +from .common import (
> > > +    build_params,
> > > +    c_enum_const,
> > > +    c_name,
> > > +    mcgen,
> > > +)
> > >   from .gen import QAPISchemaModularCVisitor, ifcontext
> > >   from .schema import QAPISchemaEnumMember
> > >   from .types import gen_enum, gen_enum_lookup
> > > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > > index 8df19a0df0..11472ba043 100644
> > > --- a/scripts/qapi/gen.py
> > > +++ b/scripts/qapi/gen.py
> > > @@ -11,13 +11,19 @@
> > >   # This work is licensed under the terms of the GNU GPL, version 2.
> > >   # See the COPYING file in the top-level directory.
> > > -
> > > +from contextlib import contextmanager
> > >   import errno
> > >   import os
> > >   import re
> > > -from contextlib import contextmanager
> > > -from .common import *
> > > +from .common import (
> > > +    c_fname,
> > > +    gen_endif,
> > > +    gen_if,
> > > +    guardend,
> > > +    guardstart,
> > > +    mcgen,
> > > +)
> > >   from .schema import QAPISchemaVisitor
> > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > > index 2a34cd1e8e..b036fcf9ce 100644
> > > --- a/scripts/qapi/introspect.py
> > > +++ b/scripts/qapi/introspect.py
> > > @@ -10,7 +10,12 @@
> > >   See the COPYING file in the top-level directory.
> > >   """
> > > -from .common import *
> > > +from .common import (
> > > +    c_name,
> > > +    gen_endif,
> > > +    gen_if,
> > > +    mcgen,
> > > +)
> > >   from .gen import QAPISchemaMonolithicCVisitor
> > >   from .schema import (QAPISchemaArrayType, QAPISchemaBuiltinType,
> > >                        QAPISchemaType)
> > > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > > index ca9a5aacb3..53b47f9e58 100644
> > > --- a/scripts/qapi/types.py
> > > +++ b/scripts/qapi/types.py
> > > @@ -13,7 +13,13 @@
> > >   # See the COPYING file in the top-level directory.
> > >   """
> > > -from .common import *
> > > +from .common import (
> > > +    c_enum_const,
> > > +    c_name,
> > > +    gen_endif,
> > > +    gen_if,
> > > +    mcgen,
> > > +)
> > >   from .gen import QAPISchemaModularCVisitor, ifcontext
> > >   from .schema import QAPISchemaEnumMember, QAPISchemaObjectType
> > > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > > index 7850f6e848..ea277e7704 100644
> > > --- a/scripts/qapi/visit.py
> > > +++ b/scripts/qapi/visit.py
> > > @@ -13,7 +13,15 @@
> > >   See the COPYING file in the top-level directory.
> > >   """
> > > -from .common import *
> > > +from .common import (
> > > +    c_enum_const,
> > > +    c_name,
> > > +    gen_endif,
> > > +    gen_if,
> > > +    mcgen,
> > > +    pop_indent,
> > > +    push_indent,
> > > +)
> > 
> > And here, isort will add the paranthesis (it does so based on space 
> > demands):
> > 
> >     from .common import (c_enum_const, c_name, gen_endif, gen_if, mcgen,
> >                          pop_indent, push_indent)
> >     from .gen import QAPISchemaModularCVisitor, ifcontext
> >     from .schema import QAPISchemaObjectType
> > 
> > Other than those suggestions, it LGTM.
> > 
> 
> OK. We can add a check that asserts that isort(file) == file to keep our
> include regimes consistent. I'll look into the tool, but it will be after
> this marathon of a series.
> 

That goes without saying!

> Here's a gitlab issue I made on my QEMU fork to help me keep track of
> Python-related issues that I intend to use:
> https://gitlab.com/jsnow/qemu/-/issues/6
> 

Nice!

- Cleber.

> > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > 
> 
> Thanks!
> 
> --js

signature.asc
Description: PGP signature