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

[John Snow]

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.

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

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

Thanks!

--js