diff mbox series

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

Message ID 20200922210101.4081073-6-jsnow@redhat.com
State Superseded
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Sept. 22, 2020, 9 p.m. UTC
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(-)

Comments

Eduardo Habkost Sept. 22, 2020, 9:37 p.m. UTC | #1
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>
Cleber Rosa Sept. 23, 2020, 1:27 p.m. UTC | #2
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>
John Snow Sept. 23, 2020, 5:21 p.m. UTC | #3
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
Cleber Rosa Sept. 24, 2020, 7:27 p.m. UTC | #4
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
John Snow Sept. 24, 2020, 8:04 p.m. UTC | #5
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
diff mbox series

Patch

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