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

[Cleber Rosa]

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

> 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.

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

signature.asc
Description: PGP signature