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

[John Snow <jsnow@redhat.com>]

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