Message ID | 20210325060356.4040114-1-jsnow@redhat.com |
---|---|
Headers | show |
Series | qapi: static typing conversion, pt3 | expand |
John Snow <jsnow@redhat.com> writes: > Hi, this series adds static types to the QAPI module. > This is part three, and it focuses on expr.py. > > Environment: > - Python >= 3.6, <= 3.8 * > - mypy >= 0.770 > - pylint >= 2.6.0 > - flake8 > - isort > > Every commit should pass with (from ./scripts/): > - flake8 qapi/ > - pylint --rcfile=qapi/pylintrc qapi/ > - mypy --config-file=qapi/mypy.ini qapi/ > - pushd qapi && isort -c . && popd > > V4: > > Patch 2 is exploratory. > Patch 8 is broken and should be merged into Patch 9. > Patches 17-19 are optional and I'd sooner you drop them than have to respin. [...] > - Add test patch to demonstrate 72col docstring enforcement. (Not a fan.) > - Changed MutableMapping type to regular ol' dict. > - Added tests for alternate and union to see what happens when we pass a list > for 'data' instead. (It crashes.) > - Rewrote a bunch of the docstrings. > - Updated type hints for rc0 > - Rebased on latest master, incorporating latest qapi changes. > - Addressed most feedback, some exceptions; > - Kept isinstance check for dict; it is strictly more convenient to me and it > does not cause breakages. It won't cause breakages. [...] I skipped PATCH 2+16+18+19. I figure these are independent enough to let me come back to it later. In case of PATCH 16, I better come back quickly, since to go and lose your doc strings would be a shame. On the other patches, I have a few questions and suggestions. So far it looks like no respin will be needed.
On 3/25/21 2:03 AM, John Snow wrote: > Hi, this series adds static types to the QAPI module. > This is part three, and it focuses on expr.py. > > Environment: > - Python >= 3.6, <= 3.8 * > - mypy >= 0.770 > - pylint >= 2.6.0 > - flake8 > - isort > > Every commit should pass with (from ./scripts/): > - flake8 qapi/ > - pylint --rcfile=qapi/pylintrc qapi/ > - mypy --config-file=qapi/mypy.ini qapi/ > - pushd qapi && isort -c . && popd > > V4: > > Patch 2 is exploratory. > Patch 8 is broken and should be merged into Patch 9. > Patches 17-19 are optional and I'd sooner you drop them than have to respin. > > 001/19:[down] 'qapi/expr: Comment cleanup' > 002/19:[down] 'flake8: Enforce shorter line length for comments and docstrings' > 003/19:[----] [--] 'qapi/expr.py: Remove 'info' argument from nested check_if_str' > 004/19:[----] [--] 'qapi/expr.py: Check for dict instead of OrderedDict' > 005/19:[0011] [FC] 'qapi/expr.py: constrain incoming expression types' > 006/19:[0006] [FC] 'qapi/expr.py: Add assertion for union type 'check_dict'' > 007/19:[----] [--] 'qapi/expr.py: move string check upwards in check_type' > 008/19:[down] 'qapi: add tests for invalid 'data' field type' > 009/19:[0004] [FC] 'qapi/expr.py: Check type of 'data' member' > 010/19:[0008] [FC] 'qapi/expr.py: Add casts in a few select cases' > 011/19:[0005] [FC] 'qapi/expr.py: Modify check_keys to accept any Collection' > 012/19:[0057] [FC] 'qapi/expr.py: add type hint annotations' > 013/19:[0032] [FC] 'qapi/expr.py: Consolidate check_if_str calls in check_if' > 014/19:[0016] [FC] 'qapi/expr.py: Remove single-letter variable' > 015/19:[----] [--] 'qapi/expr.py: enable pylint checks' > 016/19:[0168] [FC] 'qapi/expr.py: Add docstrings' > 017/19:[----] [-C] 'qapi/expr.py: Use tuples instead of lists for static data' > 018/19:[----] [-C] 'qapi/expr.py: move related checks inside check_xxx functions' > 019/19:[0003] [FC] 'qapi/expr.py: Use an expression checker dispatch table' > > - Add test patch to demonstrate 72col docstring enforcement. (Not a fan.) > - Changed MutableMapping type to regular ol' dict. > - Added tests for alternate and union to see what happens when we pass a list > for 'data' instead. (It crashes.) > - Rewrote a bunch of the docstrings. > - Updated type hints for rc0 > - Rebased on latest master, incorporating latest qapi changes. > - Addressed most feedback, some exceptions; > - Kept isinstance check for dict; it is strictly more convenient to me and it > does not cause breakages. It won't cause breakages. > > RFCs/notes: > > - I'd be flabbergasted if anyone reads these. > > John Snow (19): > qapi/expr: Comment cleanup > flake8: Enforce shorter line length for comments and docstrings > qapi/expr.py: Remove 'info' argument from nested check_if_str > qapi/expr.py: Check for dict instead of OrderedDict > qapi/expr.py: constrain incoming expression types > qapi/expr.py: Add assertion for union type 'check_dict' > qapi/expr.py: move string check upwards in check_type > qapi: add tests for invalid 'data' field type > qapi/expr.py: Check type of 'data' member > qapi/expr.py: Add casts in a few select cases > qapi/expr.py: Modify check_keys to accept any Collection > qapi/expr.py: add type hint annotations > qapi/expr.py: Consolidate check_if_str calls in check_if > qapi/expr.py: Remove single-letter variable > qapi/expr.py: enable pylint checks > qapi/expr.py: Add docstrings > qapi/expr.py: Use tuples instead of lists for static data > qapi/expr.py: move related checks inside check_xxx functions > qapi/expr.py: Use an expression checker dispatch table > > scripts/qapi/.flake8 | 1 + > scripts/qapi/common.py | 8 +- > scripts/qapi/events.py | 9 +- > scripts/qapi/expr.py | 499 +++++++++++++----- > scripts/qapi/gen.py | 8 +- > scripts/qapi/introspect.py | 8 +- > scripts/qapi/main.py | 4 +- > scripts/qapi/mypy.ini | 5 - > scripts/qapi/parser.py | 15 +- > scripts/qapi/pylintrc | 1 - > scripts/qapi/schema.py | 23 +- > scripts/qapi/types.py | 7 +- > .../alternate-invalid-data-type.err | 2 + > .../alternate-invalid-data-type.json | 4 + > .../alternate-invalid-data-type.out | 0 > tests/qapi-schema/meson.build | 2 + > tests/qapi-schema/union-invalid-data-type.err | 2 + > .../qapi-schema/union-invalid-data-type.json | 13 + > tests/qapi-schema/union-invalid-data-type.out | 0 > 19 files changed, 449 insertions(+), 162 deletions(-) > create mode 100644 tests/qapi-schema/alternate-invalid-data-type.err > create mode 100644 tests/qapi-schema/alternate-invalid-data-type.json > create mode 100644 tests/qapi-schema/alternate-invalid-data-type.out > create mode 100644 tests/qapi-schema/union-invalid-data-type.err > create mode 100644 tests/qapi-schema/union-invalid-data-type.json > create mode 100644 tests/qapi-schema/union-invalid-data-type.out > To https://gitlab.com/jsnow/qemu.git + ba5dba933a...e5f101c2f1 python-qapi-cleanup-pt3 -> python-qapi-cleanup-pt3 (forced update) Should include all of the feedback from the list today, I didn't send it back out to list to see what happens with the giant docstring patch.
On 3/25/21 2:03 AM, John Snow wrote: > Hi, this series adds static types to the QAPI module. > This is part three, and it focuses on expr.py. > > Environment: > - Python >= 3.6, <= 3.8 * > - mypy >= 0.770 > - pylint >= 2.6.0 > - flake8 > - isort > > Every commit should pass with (from ./scripts/): > - flake8 qapi/ > - pylint --rcfile=qapi/pylintrc qapi/ > - mypy --config-file=qapi/mypy.ini qapi/ > - pushd qapi && isort -c . && popd > > V4: > > Patch 2 is exploratory. > Patch 8 is broken and should be merged into Patch 9. > Patches 17-19 are optional and I'd sooner you drop them than have to respin. > > 001/19:[down] 'qapi/expr: Comment cleanup' > 002/19:[down] 'flake8: Enforce shorter line length for comments and docstrings' > 003/19:[----] [--] 'qapi/expr.py: Remove 'info' argument from nested check_if_str' > 004/19:[----] [--] 'qapi/expr.py: Check for dict instead of OrderedDict' > 005/19:[0011] [FC] 'qapi/expr.py: constrain incoming expression types' > 006/19:[0006] [FC] 'qapi/expr.py: Add assertion for union type 'check_dict'' > 007/19:[----] [--] 'qapi/expr.py: move string check upwards in check_type' > 008/19:[down] 'qapi: add tests for invalid 'data' field type' > 009/19:[0004] [FC] 'qapi/expr.py: Check type of 'data' member' > 010/19:[0008] [FC] 'qapi/expr.py: Add casts in a few select cases' > 011/19:[0005] [FC] 'qapi/expr.py: Modify check_keys to accept any Collection' > 012/19:[0057] [FC] 'qapi/expr.py: add type hint annotations' > 013/19:[0032] [FC] 'qapi/expr.py: Consolidate check_if_str calls in check_if' > 014/19:[0016] [FC] 'qapi/expr.py: Remove single-letter variable' > 015/19:[----] [--] 'qapi/expr.py: enable pylint checks' > 016/19:[0168] [FC] 'qapi/expr.py: Add docstrings' > 017/19:[----] [-C] 'qapi/expr.py: Use tuples instead of lists for static data' > 018/19:[----] [-C] 'qapi/expr.py: move related checks inside check_xxx functions' > 019/19:[0003] [FC] 'qapi/expr.py: Use an expression checker dispatch table' > > - Add test patch to demonstrate 72col docstring enforcement. (Not a fan.) > - Changed MutableMapping type to regular ol' dict. > - Added tests for alternate and union to see what happens when we pass a list > for 'data' instead. (It crashes.) > - Rewrote a bunch of the docstrings. > - Updated type hints for rc0 > - Rebased on latest master, incorporating latest qapi changes. > - Addressed most feedback, some exceptions; > - Kept isinstance check for dict; it is strictly more convenient to me and it > does not cause breakages. It won't cause breakages. > > RFCs/notes: > > - I'd be flabbergasted if anyone reads these. > > John Snow (19): > qapi/expr: Comment cleanup > flake8: Enforce shorter line length for comments and docstrings > qapi/expr.py: Remove 'info' argument from nested check_if_str > qapi/expr.py: Check for dict instead of OrderedDict > qapi/expr.py: constrain incoming expression types > qapi/expr.py: Add assertion for union type 'check_dict' > qapi/expr.py: move string check upwards in check_type > qapi: add tests for invalid 'data' field type > qapi/expr.py: Check type of 'data' member > qapi/expr.py: Add casts in a few select cases > qapi/expr.py: Modify check_keys to accept any Collection > qapi/expr.py: add type hint annotations > qapi/expr.py: Consolidate check_if_str calls in check_if > qapi/expr.py: Remove single-letter variable > qapi/expr.py: enable pylint checks > qapi/expr.py: Add docstrings > qapi/expr.py: Use tuples instead of lists for static data > qapi/expr.py: move related checks inside check_xxx functions > qapi/expr.py: Use an expression checker dispatch table > > scripts/qapi/.flake8 | 1 + > scripts/qapi/common.py | 8 +- > scripts/qapi/events.py | 9 +- > scripts/qapi/expr.py | 499 +++++++++++++----- > scripts/qapi/gen.py | 8 +- > scripts/qapi/introspect.py | 8 +- > scripts/qapi/main.py | 4 +- > scripts/qapi/mypy.ini | 5 - > scripts/qapi/parser.py | 15 +- > scripts/qapi/pylintrc | 1 - > scripts/qapi/schema.py | 23 +- > scripts/qapi/types.py | 7 +- > .../alternate-invalid-data-type.err | 2 + > .../alternate-invalid-data-type.json | 4 + > .../alternate-invalid-data-type.out | 0 > tests/qapi-schema/meson.build | 2 + > tests/qapi-schema/union-invalid-data-type.err | 2 + > .../qapi-schema/union-invalid-data-type.json | 13 + > tests/qapi-schema/union-invalid-data-type.out | 0 > 19 files changed, 449 insertions(+), 162 deletions(-) > create mode 100644 tests/qapi-schema/alternate-invalid-data-type.err > create mode 100644 tests/qapi-schema/alternate-invalid-data-type.json > create mode 100644 tests/qapi-schema/alternate-invalid-data-type.out > create mode 100644 tests/qapi-schema/union-invalid-data-type.err > create mode 100644 tests/qapi-schema/union-invalid-data-type.json > create mode 100644 tests/qapi-schema/union-invalid-data-type.out > Re-pushed to gitlab branch. We'll call this version v5.a2. - Modified the commit message for the test as per Markus' suggestion. - Expanded the comment explaining the _JSONObject thingie. --js