qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: John Snow
Subject: Re: [PATCH v2 05/38] qapi: Remove wildcard includes
Date: Thu, 24 Sep 2020 16:04:44 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 
On 9/23/20 1:21 PM, 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.
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 



I've found that
`isort --force-sort-within-sections --force-grid-wrap 4 --multi-line 3 --trailing-comma`
is pretty close to what I was already doing, so I'll adopt this for the respin on good faith that nobody will retract an R-B for new import orderings.
force sort: I prefer to sort by module, so I intersperse "from x" and "import x" style in one section. This keeps the module reference absolutely consistent regardless of HOW we import from it.
force grid: 4 or more imports from a module will get wrapped using the one-per-line style.
multi-line: '3' just refers to the one-per-line style of imports that I already use. 

trailing comma: A little buddy that hangs out with you.



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



Thanks!

--js
reply via email to
[Prev in Thread] Current Thread [Next in Thread]