1. Patchew
  2. QEMU
  3. [PATCH v2 00/38] qapi: static typing conversion, pt1
  4. View patch

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

John Snow posted 38 patches 5 years, 4 months ago
Maintainers: Eduardo Habkost <ehabkost@redhat.com>, Markus Armbruster <armbru@redhat.com>, Michael Roth <mdroth@linux.vnet.ibm.com>, Cleber Rosa <crosa@redhat.com>
There is a newer version of this series
[PATCH v2 05/38] qapi: Remove wildcard includes
Posted by John Snow 5 years, 4 months ago 
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
 
 
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,
+)
 from .gen import QAPISchemaModularCVisitor, ifcontext
 from .schema import QAPISchemaObjectType
 
-- 
2.26.2
Re: [PATCH v2 05/38] qapi: Remove wildcard includes
Posted by Eduardo Habkost 5 years, 4 months ago 
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>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo
Re: [PATCH v2 05/38] qapi: Remove wildcard includes
Posted by Cleber Rosa 5 years, 4 months ago 
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>
Re: [PATCH v2 05/38] qapi: Remove wildcard includes
Posted by John Snow 5 years, 4 months ago 
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
Re: [PATCH v2 05/38] qapi: Remove wildcard includes
Posted by Cleber Rosa 5 years, 4 months ago 
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
Re: [PATCH v2 05/38] qapi: Remove wildcard includes
Posted by John Snow 5 years, 4 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.