Hi, this series adds static types to the QAPI module. This is part three, and it focuses on expr.py.
This series is applied and hosted here: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt3 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 Please read the changelog below for some review notes that may be of interest. V3: 001/16:[----] [--] 'qapi/expr.py: Remove 'info' argument from nested check_if_str' 002/16:[----] [--] 'qapi/expr.py: Check for dict instead of OrderedDict' 003/16:[0004] [FC] 'qapi/expr.py: constrain incoming expression types' 004/16:[----] [--] 'qapi/expr.py: Add assertion for union type 'check_dict'' 005/16:[----] [--] 'qapi/expr.py: move string check upwards in check_type' 006/16:[----] [--] 'qapi/expr.py: Check type of 'data' member' 007/16:[0002] [FC] 'qapi/expr.py: Add casts in a few select cases' 008/16:[----] [--] 'qapi/expr.py: add type hint annotations' 009/16:[down] 'qapi/expr.py: Consolidate check_if_str calls in check_if' 010/16:[----] [--] 'qapi/expr.py: Remove single-letter variable' 011/16:[----] [--] 'qapi/expr.py: enable pylint checks' 012/16:[----] [-C] 'qapi/expr.py: Add docstrings' 013/16:[down] 'qapi/expr.py: Modify check_keys to accept any Collection' 014/16:[----] [--] 'qapi/expr.py: Use tuples instead of lists for static data' 015/16:[0004] [FC] 'qapi/expr.py: move related checks inside check_xxx functions' 016/16:[0011] [FC] 'qapi/expr.py: Use an expression checker dispatch table' - Some RB-s added, some dropped; see "Review Status" section below. - ("pt0" series rebased on latest origin/master.) - Rebased on origin/master. - 03: Re-ordered the Expression unpacking slightly to match the other stanzas. (R-Bs kept.) - 07: Changed capitalization of a comment. (R-Bs kept.) - 09: Rewritten more aggressively. (R-Bs dropped.) - 13: Use "Collection" instead of "Iterable" because of concerns with the possibly consumptive nature of Iterable; change commit name & message. (R-Bs dropped.) - 15: Use tuples everywhere, even for single items. (R-Bs kept.) - 16: Update ExpressionType to define a __str__ method, which allows the meta variable to be passed and used directly as a string. (R-Bs dropped.) RFCs/notes: - This series was written long before pt1.5 and pt2. Keep that in mind! (Sorry.) - I used MutableMapping instead of Dict in patch 8. There's no real reason I couldn't have used Dict, both work - this one is more abstract. Both would work for dict/OrderedDict perfectly well. (I think I had some reason at one point or another, but I can no longer remember what it is, if I am being honest. It might have to do with Dict being invariant, but MutableMapping being covariant, which might come into play much later in the six-part series. I really forget.) - The dreaded _DObject comes back, this time named _JSObject. It's a bad name. It means "Any JSON object deserialized as a Python dict". I didn't rename it because I didn't want to shed the R-Bs yet. Please suggest a name. (Or a way to avoid needing it at all.) You'll probably notice that I keep futzing with the documentation near this definition. I opted not to fix it to avoid touching patches that were (so far) fully reviewed. - Patch 12 (the docstring patch) needs to be heavily copy-edited. I figured I would simply address it all at once after review from Markus. I ask that a review of this patch be exhaustive if at all possible. I start using [Const] and [RW] markers in the summary string; I think I will actually remove these as they are not real markup, but I'd like to solicit suggestions on how to differentiate functions that modify expr from ones that do not. I also start using some fairly arbitrary syntax to try and document the syntactic forms being checked and normalized here, but they are not very consistent. Suggestions welcome. - Patch 16 is something I am not sure I really like anymore, it has some mild benefit but I don't like how I repeat the expression types twice in one file. I consider this patch optional for now. I suspect there's a neater way to write it that gives us the same benefit but looks less ugly. Review Status: [01] qapi-expr-py-remove-info # [RB] CR,EH [SOB] JS [02] qapi-expr-py-check-for-dict # [RB] CR,EH [SOB] JS [03] qapi-expr-py-constrain # [RB] CR,EH [SOB] JS [04] qapi-expr-py-add-assertion-for # [RB] CR,EH [SOB] JS [05] qapi-expr-py-move-string-check # [RB] CR,EH [SOB] JS [06] qapi-expr-py-check-type-of # [RB] EH [SOB] JS [07] qapi-expr-py-add-casts-in-a # [RB] CR,EH [SOB] JS [08] qapi-expr-py-add-type-hint # [RB] CR,EH [SOB] JS [09] qapi-expr-py-consolidate # [SOB] JS [10] qapi-expr-py-remove-single # [RB] CR,EH [SOB] JS [11] qapi-expr-py-enable-pylint # [RB] CR,EH [SOB] JS [12] qapi-expr-py-add-docstrings # [RB] CR [SOB] JS [13] qapi-expr-py-modify-check_keys # [SOB] JS [14] qapi-expr-py-use-tuples # [RB] CR,EH [SOB] JS [15] qapi-expr-py-move-related # [RB] CR [SOB] JS [16] qapi-expr-py-use-an-expression # [SOB] JS John Snow (16): 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/expr.py: Check type of 'data' member qapi/expr.py: Add casts in a few select cases 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: Modify check_keys to accept any Collection 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/expr.py | 453 +++++++++++++++++++++++++++++++----------- scripts/qapi/mypy.ini | 5 - scripts/qapi/pylintrc | 1 - 3 files changed, 337 insertions(+), 122 deletions(-) -- 2.29.2
