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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5803 bytes --]

Hi

On Mon, Nov 2, 2020 at 7:41 PM John Snow <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
> > Everything: 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?

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.

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

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


> > 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

[-- Attachment #2: Type: text/html, Size: 7715 bytes --]