Re: [PATCH v2 00/11] qapi: static typing conversion, pt2

[John Snow]

On 11/4/20 4:51 AM, Marc-André Lureau wrote:
> Hi
>
> On Mon, Nov 2, 2020 at 7:41 PM John Snow <jsnow@redhat.com 
> <mailto:jsnow@redhat.com>> wrote:
>
>     On 10/26/20 3:42 PM, John Snow wrote:
>      > Hi, this series adds static type hints to the QAPI module.
>      > This is part two, and covers introspect.py.
>      >
>      > Part 2:
>     https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2
>     <https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2>
>      > Everything:
>     https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6
>     <https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6>
>      >
>      > - Requires Python 3.6+
>      > - Requires mypy 0.770 or newer (for type analysis only)
>      > - Requires pylint 2.6.0 or newer (for lint checking only)
>      >
>      > Type hints are added in patches that add *only* type hints and
>     change no
>      > other behavior. Any necessary changes to behavior to accommodate
>     typing
>      > are split out into their own tiny patches.
>      >
>      > Every commit should pass with:
>      >   - flake8 qapi/
>      >   - pylint --rcfile=qapi/pylintrc qapi/
>      >   - mypy --config-file=qapi/mypy.ini qapi/
>      >
>      > V2:
>      >   - Dropped all R-B from previous series; enough has changed.
>      >   - pt2 is now introspect.py, expr.py is pushed to pt3.
>      >   - Reworked again to have less confusing (?) type names
>      >   - Added an assertion to prevent future accidental breakage
>      >
>
>     Ping!
>
>     Patches 1-3: Can be skipped; just enables sphinx to check the docstring
>     syntax. Don't worry about these too much, they're just here for you to
>     test with.
>
>
> They are interesting, but the rebase version fails. And the error 
> produced is not exactly friendly:
> Exception occurred:
>    File "/usr/lib/python3.9/site-packages/sphinx/domains/c.py", line 
> 3751, in resolve_any_xref
>      return [('c:' + self.role_for_objtype(objtype), retnode)]
> TypeError: can only concatenate str (not "NoneType") to str
>
> Could you rebase and split off in a separate series?

Done, new versions of these patches will omit these.

>     Patch 4 adds some small changes, to support:
>     Patch 5 adds the type hints.
>     Patches 6-11 try to improve the readability of the types and the code.
>
>     This was a challenging file to clean up, so I am sure there's lots of
>     easy, low-hanging fruit in the review/feedback for me to improve.
>
>
> Nothing obvious to me.
>
> Python typing is fairly new to me, and I don't know the best practices. 
> I would just take what you did and improve later, if needed.

Neither do I, but I'm learning as I go.

> A wish item before we proceed with more python code cleanups is 
> documentation and/or automated tests.

OK. It's on my list to write a python style guide document, I can detail 
our typing strategies, documentation strategies, etc there.

> Could you add a new make check-python and perhaps document what the new 
> code-style expectations?

Yes, I have one for python/qemu that I tried to get merged prior to 5.2 
but it didn't make it in time because there were some concerns over 
exactly how the test ran and where it provisioned its packages from.

>      > John Snow (11):
>      >    [DO-NOT-MERGE] docs: replace single backtick (`) with
>     double-backtick
>      >      (``)
>      >    [DO-NOT-MERGE] docs/sphinx: change default role to "any"
>      >    [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi
>      >    qapi/introspect.py: add assertions and casts
>      >    qapi/introspect.py: add preliminary type hint annotations
>      >    qapi/introspect.py: add _gen_features helper
>      >    qapi/introspect.py: Unify return type of _make_tree()
>      >    qapi/introspect.py: replace 'extra' dict with 'comment' argument
>      >    qapi/introspect.py: create a typed 'Annotated' data strutcure
>      >    qapi/introspect.py: improve readability of _tree_to_qlit
>      >    qapi/introspect.py: Add docstring to _tree_to_qlit
>      >
>      >   docs/conf.py                           |   6 +-
>      >   docs/devel/build-system.rst            | 120 +++++------
>      >   docs/devel/index.rst                   |   1 +
>      >   docs/devel/migration.rst               |  59 +++---
>      >   docs/devel/python/index.rst            |   7 +
>      >   docs/devel/python/qapi.commands.rst    |   7 +
>      >   docs/devel/python/qapi.common.rst      |   7 +
>      >   docs/devel/python/qapi.error.rst       |   7 +
>      >   docs/devel/python/qapi.events.rst      |   7 +
>      >   docs/devel/python/qapi.expr.rst        |   7 +
>      >   docs/devel/python/qapi.gen.rst         |   7 +
>      >   docs/devel/python/qapi.introspect.rst  |   7 +
>      >   docs/devel/python/qapi.main.rst        |   7 +
>      >   docs/devel/python/qapi.parser.rst      |   8 +
>      >   docs/devel/python/qapi.rst             |  26 +++
>      >   docs/devel/python/qapi.schema.rst      |   7 +
>      >   docs/devel/python/qapi.source.rst      |   7 +
>      >   docs/devel/python/qapi.types.rst       |   7 +
>      >   docs/devel/python/qapi.visit.rst       |   7 +
>      >   docs/devel/tcg-plugins.rst             |  14 +-
>      >   docs/devel/testing.rst                 |   2 +-
>      >   docs/interop/live-block-operations.rst |   4 +-
>      >   docs/system/arm/cpu-features.rst       | 110 +++++-----
>      >   docs/system/arm/nuvoton.rst            |   2 +-
>      >   docs/system/s390x/protvirt.rst         |  10 +-
>      >   qapi/block-core.json                   |   4 +-
>      >   scripts/qapi/introspect.py             | 277
>     +++++++++++++++++--------
>      >   scripts/qapi/mypy.ini                  |   5 -
>      >   scripts/qapi/schema.py                 |   2 +-
>      >   29 files changed, 487 insertions(+), 254 deletions(-)
>      >   create mode 100644 docs/devel/python/index.rst
>      >   create mode 100644 docs/devel/python/qapi.commands.rst
>      >   create mode 100644 docs/devel/python/qapi.common.rst
>      >   create mode 100644 docs/devel/python/qapi.error.rst
>      >   create mode 100644 docs/devel/python/qapi.events.rst
>      >   create mode 100644 docs/devel/python/qapi.expr.rst
>      >   create mode 100644 docs/devel/python/qapi.gen.rst
>      >   create mode 100644 docs/devel/python/qapi.introspect.rst
>      >   create mode 100644 docs/devel/python/qapi.main.rst
>      >   create mode 100644 docs/devel/python/qapi.parser.rst
>      >   create mode 100644 docs/devel/python/qapi.rst
>      >   create mode 100644 docs/devel/python/qapi.schema.rst
>      >   create mode 100644 docs/devel/python/qapi.source.rst
>      >   create mode 100644 docs/devel/python/qapi.types.rst
>      >   create mode 100644 docs/devel/python/qapi.visit.rst
>      >
>
>
>
>
> --
> Marc-André Lureau