Re: [PATCH v5 00/36] qapi: static typing conversion, pt1

2020-10-07 Thread Markus Armbruster 
John Snow  writes:

> Hi, this series adds static type hints to the QAPI module.
> This is part one!
>
> Part 1: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt1
> 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)
>
> In general, this series tackles the cleanup of one individual QAPI
> module at a time. Once it passes pylint or mypy checks, those checks are
> enabled for that file.
>
> 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.

Neat and pleasant to review.  The care you put into this shows.  Thanks!

Re: [PATCH v5 00/36] qapi: static typing conversion, pt1

2020-10-06 Thread Cleber Rosa 
On Mon, Oct 05, 2020 at 07:57:18PM -0400, John Snow wrote:
> On 10/5/20 7:05 PM, Cleber Rosa wrote:
> > Here on patches 08 an later, I've run:
> > 
> >flake8 ./scripts/qapi
> > 
> > And starting on patch 24 ("qapi/source.py: add type hint annotations")
> > it complained about:
> > 
> >/scripts/qapi/source.py:44:31: F821 undefined name 'T'
> 
> I think that must be flake8 < 3.8.0. Try using >= 3.8.0.
>

ACK, I was on 3.7.9.  They're all passing now with 3.8.4.

- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v5 00/36] qapi: static typing conversion, pt1

2020-10-05 Thread John Snow 

On 10/5/20 7:05 PM, Cleber Rosa wrote:

Here on patches 08 an later, I've run:

   flake8 ./scripts/qapi

And starting on patch 24 ("qapi/source.py: add type hint annotations")
it complained about:

   /scripts/qapi/source.py:44:31: F821 undefined name 'T'


I think that must be flake8 < 3.8.0. Try using >= 3.8.0.

(Yes, still working on getting proper pipenv/requirements/something up 
for these, sorry.)

Re: [PATCH v5 00/36] qapi: static typing conversion, pt1

2020-10-05 Thread Cleber Rosa 
On Mon, Oct 05, 2020 at 03:51:22PM -0400, John Snow wrote:
> Hi, this series adds static type hints to the QAPI module.
> This is part one!
> 
> Part 1: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt1
> 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)
> 
> In general, this series tackles the cleanup of one individual QAPI
> module at a time. Once it passes pylint or mypy checks, those checks are
> enabled for that file.
> 
> 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.
> 
> Notes:
> 
> - After patch 07, `isort -c` should pass 100% on this and every
>   future commit.
>

FIY, I went on to replicate/validate your testing, and I ran on
patches 07 an later:

  isort --check-only --settings-path ./scripts/qapi/.isort.cfg --recursive 
./scripts/qapi

With success.

> - After patch 08, `flake8 qapi/` should pass 100% on this and every
>   future commit.
>

Here on patches 08 an later, I've run:

  flake8 ./scripts/qapi

And starting on patch 24 ("qapi/source.py: add type hint annotations")
it complained about:

  /scripts/qapi/source.py:44:31: F821 undefined name 'T'

> - After patch 09, `pylint --rcfile=qapi/pylintrc qapi/` should pass 100%
>   on this and every future commit.
>

Here I ran on patches 09 and later:

  pylint --rcfile=./scripts/qapi/pylintrc ./scripts/qapi/

And all succeeded.

> - After patch 18, `mypy --config-file=qapi/mypy.ini qapi/` should pass
>   100% on this and every future commit.
>

Here I ran on patches 18 and later:

  mypy --config-file=./scripts/qapi/mypy.ini ./scripts/qapi/

And all succeeded.

- Cleber.


signature.asc
Description: PGP signature

[PATCH v5 00/36] qapi: static typing conversion, pt1

2020-10-05 Thread John Snow 
Hi, this series adds static type hints to the QAPI module.
This is part one!

Part 1: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt1
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)

In general, this series tackles the cleanup of one individual QAPI
module at a time. Once it passes pylint or mypy checks, those checks are
enabled for that file.

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.

Notes:

- After patch 07, `isort -c` should pass 100% on this and every
  future commit.

- After patch 08, `flake8 qapi/` should pass 100% on this and every
  future commit.

- After patch 09, `pylint --rcfile=qapi/pylintrc qapi/` should pass 100%
  on this and every future commit.

- After patch 18, `mypy --config-file=qapi/mypy.ini qapi/` should pass
  100% on this and every future commit.

Review Status:

[01] docs-repair-broken-references  #
[02] qapi-modify-docstrings-to-be   #
[03] qapi-gen-separate-arg-parsing  # [RB] CR,EH [TB] CR [SOB] JS
[04] qapi-move-generator-entrypoint # [RB] CR,EH [TB] CR [SOB] JS
[05] qapi-prefer-explicit-relative  # [RB] CR,EH [SOB] JS
[06] qapi-remove-wildcard-includes  # [RB] CR,EH [SOB] JS
[07] qapi-enforce-import-order  # [RB] CR [TB] CR [SOB] JS
[08] qapi-delint-using-flake8   # [RB] CR,EH [SOB] JS
[09] qapi-add-pylintrc  # [RB] CR [TB] CR,EH [SOB] JS
[10] qapi-common-py-remove-python   # [RB] CR,EH [SOB] JS
[11] qapi-common-add-indent-manager # [RB] CR,EH [SOB] JS
[12] qapi-common-py-delint-with # [RB] CR,EH [SOB] JS
[13] replace-c-by-char  # [RB] CR,EH [SOB] JS
[14] qapi-common-py-check-with  # [RB] CR [TB] CR,EH [SOB] JS
[15] qapi-common-py-add-notational  # [RB] CR,EH [SOB] JS
[16] qapi-common-move-comments-into # [RB] CR,EH [SOB] JS
[17] qapi-split-build_params-into   # [RB] CR,EH [SOB] JS
[18] qapi-establish-mypy-type   # [RB] CR [TB] CR,EH [SOB] JS
[19] qapi-events-py-add-notational  # [RB] CR,EH [SOB] JS
[20] qapi-events-move-comments-into # [RB] CR,EH [SOB] JS
[21] qapi-commands-py-don-t-re-bind # [RB] CR,EH [SOB] JS
[22] qapi-commands-py-add   # [RB] CR,EH [SOB] JS
[23] qapi-commands-py-enable# [RB] CR,EH [SOB] JS
[24] qapi-source-py-add-notational  # [RB] CR,EH [TB] CR [SOB] JS
[25] qapi-source-py-delint-with # [RB] CR,EH [TB] CR [SOB] JS
[26] qapi-gen-py-fix-edge-case-of   # [RB] CR,EH [SOB] JS
[27] qapi-gen-py-add-notational # [RB] CR,EH [SOB] JS
[28] qapi-gen-py-enable-checking# [RB] CR,EH [TB] CR [SOB] JS
[29] qapi-gen-py-remove-unused  # [RB] CR,EH [SOB] JS
[30] qapi-gen-py-update-write-to-be # [RB] CR,EH [SOB] JS
[31] qapi-gen-py-delint-with-pylint # [RB] CR,EH [SOB] JS
[32] qapi-types-py-add-type-hint# [RB] CR,EH [SOB] JS
[33] qapi-types-py-remove-one   # [RB] CR,EH [SOB] JS
[34] qapi-visit-py-assert   # [RB] CR,EH [SOB] JS
[35] qapi-visit-py-remove-unused# [RB] CR,EH [TB] CR [SOB] JS
[36] qapi-visit-py-add-notational   # [RB] CR,EH [TB] CR [SOB] JS

Changelog:

002/36:[0012] [FC] 'qapi: modify docstrings to be sphinx-compatible'
027/36:[] [-C] 'qapi/gen.py: add type hint annotations'

V5:
 - Remove DO-NOT-MERGE patches (Now in Part2)
 - Remove introspect.py patches (Now in Part2)
 - 02: Docstring formatting, more commit message (Markus)

V4:
 - Rebase on Peter Maydell's work;
  - 05: Context differences from Peter Maydell's work
  - 06: Remove qapi.doc
  - 07: Remove qapi.doc, remove superfluous "if match"
  - 09: Remove qapi.doc
  - 13: remove qapi.doc
  - 18: remove qapi.doc
  - 22: remove qapi.doc
  - 31: remove QAPIGenDoc changes

 - Minor adjustments from list feedback;
  - 01: More backticks for QAPI json files, now that they are in Sphinx-exe
  - 02: Use :ref: role for the `docker-ref` cross-reference
  - 04: Remove doc.py work changes; add references for start_if/end_if
  - 10: Changes to accommodate isort
  - 11: Added lines_after_imports=2
  - 16: isort whitespace changes
  - 24: Take Markus's docstring phrasing
  - 34: add comment explaining os.open()
  - 37: isort differences

V3:
 - Use isort to enforce import consistency
 - Use sphinx apidoc to check docstring format

V2:
 - Removed Python3.6 patch in favor of Thomas Huth's
 - Addressed (most) feedback from Markus
 - Renamed type hint annotation commits

John Snow (36):
  docs: repair broken references
  qapi: modify docstrings to be sphinx-compatible
  qapi-gen: Separate arg-parsing from generation
  qapi: move generator entrypoint into module
  qapi: Prefer explicit relative imports
  qapi: Remove wildcard includes
  qapi: enforce import order/styling with isort
  qapi: delint using flake8
  qapi: add pylintrc
  qapi/common.py: Remove python compatibility workaround
  qapi/common.py: