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 -- 2.30.2
Fixes: 0825f62c842 Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 540b3982b1..c207481f7e 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -241,7 +241,7 @@ def check_enum(expr, info): source = "%s '%s'" % (source, member_name) # Enum members may start with a digit if member_name[0].isdigit(): - member_name = 'd' + member_name # Hack: hide the digit + member_name = 'd' + member_name # Hack: hide the digit check_name_lower(member_name, info, source, permit_upper=permissive, permit_underscore=permissive) -- 2.30.2
PEP8's BDFL writes: "For flowing long blocks of text with fewer structural restrictions (docstrings or comments), the line length should be limited to 72 characters." I do not like this patch. I have included it explicitly to recommend we do not pay any further heed to the 72 column limit. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/.flake8 | 1 + scripts/qapi/common.py | 8 +++++--- scripts/qapi/events.py | 9 +++++---- scripts/qapi/gen.py | 8 ++++---- scripts/qapi/introspect.py | 8 +++++--- scripts/qapi/main.py | 4 ++-- scripts/qapi/parser.py | 15 ++++++++------- scripts/qapi/schema.py | 23 +++++++++++++---------- scripts/qapi/types.py | 7 ++++--- 9 files changed, 47 insertions(+), 36 deletions(-) diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8 index 6b158c68b8..4f00455290 100644 --- a/scripts/qapi/.flake8 +++ b/scripts/qapi/.flake8 @@ -1,2 +1,3 @@ [flake8] extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's +max-doc-length = 72 \ No newline at end of file diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index cbd3fd81d3..6e3d9b8ecd 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -41,7 +41,8 @@ def camel_to_upper(value: str) -> str: length = len(c_fun_str) for i in range(length): char = c_fun_str[i] - # When char is upper case and no '_' appears before, do more checks + # When char is upper case and no '_' appears before, + # do more checks if char.isupper() and (i > 0) and c_fun_str[i - 1] != '_': if i < length - 1 and c_fun_str[i + 1].islower(): new_name += '_' @@ -78,8 +79,9 @@ def c_name(name: str, protect: bool = True) -> str: protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' :param name: The name to map. - :param protect: If true, avoid returning certain ticklish identifiers - (like C keywords) by prepending ``q_``. + :param protect: If true, avoid returning certain ticklish + identifiers (like C keywords) by prepending + ``q_``. """ # ANSI X3J11/88-090, 3.1.1 c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py index fee8c671e7..210b56974f 100644 --- a/scripts/qapi/events.py +++ b/scripts/qapi/events.py @@ -48,7 +48,8 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str: """ Generate a struct variable holding the event parameters. - Initialize it with the function arguments defined in `gen_event_send`. + Initialize it with the function arguments defined in + `gen_event_send`. """ assert not typ.variants ret = mcgen(''' @@ -86,9 +87,9 @@ def gen_event_send(name: str, # FIXME: Our declaration of local variables (and of 'errp' in the # parameter list) can collide with exploded members of the event's # data type passed in as parameters. If this collision ever hits in - # practice, we can rename our local variables with a leading _ prefix, - # or split the code into a wrapper function that creates a boxed - # 'param' object then calls another to do the real work. + # practice, we can rename our local variables with a leading _ + # prefix, or split the code into a wrapper function that creates a + # boxed 'param' object then calls another to do the real work. have_args = boxed or (arg_type and not arg_type.is_empty()) ret = mcgen(''' diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index 1fa503bdbd..c54980074e 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -63,9 +63,9 @@ def _bottom(self) -> str: return '' def write(self, output_dir: str) -> None: - # Include paths starting with ../ are used to reuse modules of the main - # schema in specialised schemas. Don't overwrite the files that are - # already generated for the main schema. + # Include paths starting with ../ are used to reuse modules + # of the main schema in specialised schemas. Don't overwrite + # the files that are already generated for the main schema. if self.fname.startswith('../'): return pathname = os.path.join(output_dir, self.fname) @@ -189,7 +189,7 @@ def _bottom(self) -> str: @contextmanager def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]: """ - A with-statement context manager that wraps with `start_if()` / `end_if()`. + A context manager that wraps output with `start_if()` / `end_if()`. :param ifcond: A sequence of conditionals, passed to `start_if()`. :param args: any number of `QAPIGenCCode`. diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 9a348ca2e5..faf00013ad 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -61,8 +61,9 @@ # With optional annotations, the type of all values is: # JSONValue = Union[_Value, Annotated[_Value]] # -# Sadly, mypy does not support recursive types; so the _Stub alias is used to -# mark the imprecision in the type model where we'd otherwise use JSONValue. +# Sadly, mypy does not support recursive types; so the _Stub alias is +# used to mark the imprecision in the type model where we'd otherwise +# use JSONValue. _Stub = Any _Scalar = Union[str, bool, None] _NonScalar = Union[Dict[str, _Stub], List[_Stub]] @@ -217,7 +218,8 @@ def visit_end(self) -> None: self._name_map = {} def visit_needed(self, entity: QAPISchemaEntity) -> bool: - # Ignore types on first pass; visit_end() will pick up used types + # Ignore types on first pass; + # visit_end() will pick up used types return not isinstance(entity, QAPISchemaType) def _name(self, name: str) -> str: diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py index 703e7ed1ed..5bcac83985 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -1,5 +1,5 @@ -# This work is licensed under the terms of the GNU GPL, version 2 or later. -# See the COPYING file in the top-level directory. +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. """ QAPI Generator diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 58267c3db9..d5bf91f2b0 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -331,8 +331,8 @@ def __init__(self, parser, name=None, indent=0): self._indent = indent def append(self, line): - # Strip leading spaces corresponding to the expected indent level - # Blank lines are always OK. + # Strip leading spaces corresponding to the expected indent + # level. Blank lines are always OK. if line: indent = re.match(r'\s*', line).end() if indent < self._indent: @@ -353,10 +353,10 @@ def connect(self, member): self.member = member def __init__(self, parser, info): - # self._parser is used to report errors with QAPIParseError. The - # resulting error position depends on the state of the parser. - # It happens to be the beginning of the comment. More or less - # servicable, but action at a distance. + # self._parser is used to report errors with QAPIParseError. + # The resulting error position depends on the state of the + # parser. It happens to be the beginning of the comment. More + # or less servicable, but action at a distance. self._parser = parser self.info = info self.symbol = None @@ -430,7 +430,8 @@ def _append_body_line(self, line): if not line.endswith(':'): raise QAPIParseError(self._parser, "line should end with ':'") self.symbol = line[1:-1] - # FIXME invalid names other than the empty string aren't flagged + # FIXME invalid names other than the empty string aren't + # flagged if not self.symbol: raise QAPIParseError(self._parser, "invalid name") elif self.symbol: diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 703b446fd2..01cdd753cd 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -166,9 +166,10 @@ def is_user_module(cls, name: str) -> bool: @classmethod def is_builtin_module(cls, name: str) -> bool: """ - The built-in module is a single System module for the built-in types. + Return true when given the built-in module name. - It is always "./builtin". + The built-in module is a specific System module for the built-in + types. It is always "./builtin". """ return name == cls.BUILTIN_MODULE_NAME @@ -294,7 +295,8 @@ def connect_doc(self, doc=None): m.connect_doc(doc) def is_implicit(self): - # See QAPISchema._make_implicit_enum_type() and ._def_predefineds() + # See QAPISchema._make_implicit_enum_type() and + # ._def_predefineds() return self.name.endswith('Kind') or self.name == 'QType' def c_type(self): @@ -421,9 +423,9 @@ def check(self, schema): self.members = members # mark completed - # Check that the members of this type do not cause duplicate JSON members, - # and update seen to track the members seen so far. Report any errors - # on behalf of info, which is not necessarily self.info + # Check that the members of this type do not cause duplicate JSON + # members, and update seen to track the members seen so far. Report + # any errors on behalf of info, which is not necessarily self.info def check_clash(self, info, seen): assert self._checked assert not self.variants # not implemented @@ -494,11 +496,12 @@ def __init__(self, name, info, doc, ifcond, features, variants): def check(self, schema): super().check(schema) self.variants.tag_member.check(schema) - # Not calling self.variants.check_clash(), because there's nothing - # to clash with + # Not calling self.variants.check_clash(), because there's + # nothing to clash with self.variants.check(schema, {}) - # Alternate branch names have no relation to the tag enum values; - # so we have to check for potential name collisions ourselves. + # Alternate branch names have no relation to the tag enum + # values; so we have to check for potential name collisions + # ourselves. seen = {} types_seen = {} for v in self.variants.variants: diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 20d572a23a..2e67ab1752 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -35,8 +35,8 @@ from .source import QAPISourceInfo -# variants must be emitted before their container; track what has already -# been output +# variants must be emitted before their container; track what has +# already been output objects_seen = set() @@ -297,7 +297,8 @@ def _begin_user_module(self, name: str) -> None: ''')) def visit_begin(self, schema: QAPISchema) -> None: - # gen_object() is recursive, ensure it doesn't visit the empty type + # gen_object() is recursive, ensure + # it doesn't visit the empty type objects_seen.add(schema.the_empty_object_type.name) def _gen_type_cleanup(self, name: str) -> None: -- 2.30.2
The function can just use the argument from the scope above. Otherwise, we get shadowed argument errors because the parameter name clashes with the name of a variable already in-scope. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Cleber Rosa <crosa@redhat.com> --- scripts/qapi/expr.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index c207481f7e..3fda5d5082 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -122,7 +122,7 @@ def check_flags(expr, info): def check_if(expr, info, source): - def check_if_str(ifcond, info): + def check_if_str(ifcond): if not isinstance(ifcond, str): raise QAPISemError( info, @@ -142,9 +142,9 @@ def check_if_str(ifcond, info): raise QAPISemError( info, "'if' condition [] of %s is useless" % source) for elt in ifcond: - check_if_str(elt, info) + check_if_str(elt) else: - check_if_str(ifcond, info) + check_if_str(ifcond) expr['if'] = [ifcond] -- 2.30.2
OrderedDict is a subtype of dict, so we can check for a more general form. These functions do not themselves depend on it being any particular type. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Cleber Rosa <crosa@redhat.com> --- scripts/qapi/expr.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 3fda5d5082..b4bbcd54c0 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -14,7 +14,6 @@ # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. -from collections import OrderedDict import re from .common import c_name @@ -149,7 +148,7 @@ def check_if_str(ifcond): def normalize_members(members): - if isinstance(members, OrderedDict): + if isinstance(members, dict): for key, arg in members.items(): if isinstance(arg, dict): continue @@ -180,7 +179,7 @@ def check_type(value, info, source, if not allow_dict: raise QAPISemError(info, "%s should be a type name" % source) - if not isinstance(value, OrderedDict): + if not isinstance(value, dict): raise QAPISemError(info, "%s should be an object or type name" % source) -- 2.30.2
mypy does not know the types of values stored in Dicts that masquerade as objects. Help the type checker out by constraining the type. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index b4bbcd54c0..b75c85c160 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -15,9 +15,18 @@ # See the COPYING file in the top-level directory. import re +from typing import Dict, Optional from .common import c_name from .error import QAPISemError +from .parser import QAPIDoc +from .source import QAPISourceInfo + + +# Deserialized JSON objects as returned by the parser; +# The values of this mapping are not necessary to exhaustively type +# here, because the purpose of this module is to interrogate that type. +_JSONObject = Dict[str, object] # Names consist of letters, digits, -, and _, starting with a letter. @@ -315,9 +324,20 @@ def check_event(expr, info): def check_exprs(exprs): for expr_elem in exprs: - expr = expr_elem['expr'] - info = expr_elem['info'] - doc = expr_elem.get('doc') + # Expression + assert isinstance(expr_elem['expr'], dict) + for key in expr_elem['expr'].keys(): + assert isinstance(key, str) + expr: _JSONObject = expr_elem['expr'] + + # QAPISourceInfo + assert isinstance(expr_elem['info'], QAPISourceInfo) + info: QAPISourceInfo = expr_elem['info'] + + # Optional[QAPIDoc] + tmp = expr_elem.get('doc') + assert tmp is None or isinstance(tmp, QAPIDoc) + doc: Optional[QAPIDoc] = tmp if 'include' in expr: continue -- 2.30.2
mypy isn't fond of allowing you to check for bool membership in a collection of str elements. Guard this lookup for precisely when we were given a name. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Cleber Rosa <crosa@redhat.com> --- scripts/qapi/expr.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index b75c85c160..2a2cf7064f 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -192,7 +192,9 @@ def check_type(value, info, source, raise QAPISemError(info, "%s should be an object or type name" % source) - permissive = allow_dict in info.pragma.member_name_exceptions + permissive = False + if isinstance(allow_dict, str): + permissive = allow_dict in info.pragma.member_name_exceptions # value is a dictionary, check that each member is okay for (key, arg) in value.items(): -- 2.30.2
For readability purposes only, shimmy the early return upwards to the top of the function, so cases proceed in order from least to most complex. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Cleber Rosa <crosa@redhat.com> --- scripts/qapi/expr.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 2a2cf7064f..73e7d8cb0d 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -169,6 +169,10 @@ def check_type(value, info, source, if value is None: return + # Type name + if isinstance(value, str): + return + # Array type if isinstance(value, list): if not allow_array: @@ -179,10 +183,6 @@ def check_type(value, info, source, source) return - # Type name - if isinstance(value, str): - return - # Anonymous type if not allow_dict: -- 2.30.2
It needs to be an object (dict), not anything else. Signed-off-by: John Snow <jsnow@redhat.com> --- Note: this actually doesn't ... work, but on-list, we discussed wanting tests first, then the fix. That can't happen here, because QAPI crashes at runtime. So uh, just squash this into the following patch, I guess? I tried. Signed-off-by: John Snow <jsnow@redhat.com> --- tests/qapi-schema/alternate-invalid-data-type.err | 0 tests/qapi-schema/alternate-invalid-data-type.json | 4 ++++ tests/qapi-schema/alternate-invalid-data-type.out | 0 tests/qapi-schema/meson.build | 2 ++ tests/qapi-schema/union-invalid-data-type.err | 0 tests/qapi-schema/union-invalid-data-type.json | 13 +++++++++++++ tests/qapi-schema/union-invalid-data-type.out | 0 7 files changed, 19 insertions(+) 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 diff --git a/tests/qapi-schema/alternate-invalid-data-type.err b/tests/qapi-schema/alternate-invalid-data-type.err new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/alternate-invalid-data-type.json b/tests/qapi-schema/alternate-invalid-data-type.json new file mode 100644 index 0000000000..7d5d905581 --- /dev/null +++ b/tests/qapi-schema/alternate-invalid-data-type.json @@ -0,0 +1,4 @@ +# Alternate type requires an object for 'data' +{ 'alternate': 'Alt', + 'data': ['rubbish', 'nonsense'] +} diff --git a/tests/qapi-schema/alternate-invalid-data-type.out b/tests/qapi-schema/alternate-invalid-data-type.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index 8ba6917132..cc5b136cfb 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -15,6 +15,7 @@ schemas = [ 'alternate-conflict-bool-string.json', 'alternate-conflict-num-string.json', 'alternate-empty.json', + 'alternate-invalid-data-type.json', 'alternate-invalid-dict.json', 'alternate-nested.json', 'alternate-unknown.json', @@ -192,6 +193,7 @@ schemas = [ 'union-clash-branches.json', 'union-empty.json', 'union-invalid-base.json', + 'union-invalid-data-type.json', 'union-optional-branch.json', 'union-unknown.json', 'unknown-escape.json', diff --git a/tests/qapi-schema/union-invalid-data-type.err b/tests/qapi-schema/union-invalid-data-type.err new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/union-invalid-data-type.json b/tests/qapi-schema/union-invalid-data-type.json new file mode 100644 index 0000000000..5a32d267bf --- /dev/null +++ b/tests/qapi-schema/union-invalid-data-type.json @@ -0,0 +1,13 @@ +# the union data type must be an object. + +{ 'struct': 'TestTypeA', + 'data': { 'string': 'str' } } + +{ 'struct': 'TestTypeB', + 'data': { 'integer': 'int' } } + +{ 'union': 'TestUnion', + 'base': 'int', + 'discriminator': 'int', + 'data': ['TestTypeA', 'TestTypeB'] +} diff --git a/tests/qapi-schema/union-invalid-data-type.out b/tests/qapi-schema/union-invalid-data-type.out new file mode 100644 index 0000000000..e69de29bb2 -- 2.30.2
We don't actually check, so the user can get some unpleasant stacktraces. Formalize it. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 7 +++++++ tests/qapi-schema/alternate-invalid-data-type.err | 2 ++ tests/qapi-schema/union-invalid-data-type.err | 2 ++ 3 files changed, 11 insertions(+) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 73e7d8cb0d..ca5ab7bfda 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -281,6 +281,9 @@ def check_union(expr, info): raise QAPISemError(info, "'discriminator' requires 'base'") check_name_is_str(discriminator, info, "'discriminator'") + if not isinstance(members, dict): + raise QAPISemError(info, "'data' must be an object") + for (key, value) in members.items(): source = "'data' member '%s'" % key if discriminator is None: @@ -296,6 +299,10 @@ def check_alternate(expr, info): if not members: raise QAPISemError(info, "'data' must not be empty") + + if not isinstance(members, dict): + raise QAPISemError(info, "'data' must be an object") + for (key, value) in members.items(): source = "'data' member '%s'" % key check_name_lower(key, info, source) diff --git a/tests/qapi-schema/alternate-invalid-data-type.err b/tests/qapi-schema/alternate-invalid-data-type.err index e69de29bb2..c7301ccb00 100644 --- a/tests/qapi-schema/alternate-invalid-data-type.err +++ b/tests/qapi-schema/alternate-invalid-data-type.err @@ -0,0 +1,2 @@ +alternate-invalid-data-type.json: In alternate 'Alt': +alternate-invalid-data-type.json:2: 'data' must be an object diff --git a/tests/qapi-schema/union-invalid-data-type.err b/tests/qapi-schema/union-invalid-data-type.err index e69de29bb2..b71c3400c5 100644 --- a/tests/qapi-schema/union-invalid-data-type.err +++ b/tests/qapi-schema/union-invalid-data-type.err @@ -0,0 +1,2 @@ +union-invalid-data-type.json: In union 'TestUnion': +union-invalid-data-type.json:9: 'data' must be an object -- 2.30.2
Casts are instructions to the type checker only, they aren't "safe" and should probably be avoided in general. In this case, when we perform type checking on a nested structure, the type of each field does not "stick". (See PEP 647 for an example of "type narrowing" that does "stick". It is available in Python 3.10, so we can't use it yet.) We don't need to assert that something is a str if we've already checked or asserted that it is -- use a cast instead for these cases. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index ca5ab7bfda..505e67bd21 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -15,7 +15,7 @@ # See the COPYING file in the top-level directory. import re -from typing import Dict, Optional +from typing import Dict, Optional, cast from .common import c_name from .error import QAPISemError @@ -259,7 +259,7 @@ def check_enum(expr, info): def check_struct(expr, info): - name = expr['struct'] + name = cast(str, expr['struct']) # Asserted in check_exprs members = expr['data'] check_type(members, info, "'data'", allow_dict=name) @@ -267,7 +267,7 @@ def check_struct(expr, info): def check_union(expr, info): - name = expr['union'] + name = cast(str, expr['union']) # Asserted in check_exprs base = expr.get('base') discriminator = expr.get('discriminator') members = expr['data'] @@ -366,8 +366,8 @@ def check_exprs(exprs): else: raise QAPISemError(info, "expression is missing metatype") - name = expr[meta] - check_name_is_str(name, info, "'%s'" % meta) + check_name_is_str(expr[meta], info, "'%s'" % meta) + name = cast(str, expr[meta]) info.set_defn(meta, name) check_defn_name_str(name, info, meta) -- 2.30.2
This is a minor adjustment that allows the 'required' and 'optional' keys fields to take a default value of an empty, immutable sequence (the empty tuple). This reveals a quirk of this function, which is that "a + b" is list-specific behavior. We can accept a wider variety of types if we avoid that behavior. Using Collection allows us to accept things like lists, tuples, sets, and so on. (Iterable would also have worked, but Iterable also includes things like generator expressions which are consumed upon iteration, which would require a rewrite to make sure that each input was only traversed once.) Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 505e67bd21..7e22723b50 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -100,7 +100,7 @@ def pprint(elems): "%s misses key%s %s" % (source, 's' if len(missing) > 1 else '', pprint(missing))) - allowed = set(required + optional) + allowed = set(required) | set(optional) unknown = set(value) - allowed if unknown: raise QAPISemError( -- 2.30.2
Annotations do not change runtime behavior. This commit *only* adds annotations. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 68 +++++++++++++++++++++++++++---------------- scripts/qapi/mypy.ini | 5 ---- 2 files changed, 43 insertions(+), 30 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 7e22723b50..ea9d39fcf2 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -15,7 +15,15 @@ # See the COPYING file in the top-level directory. import re -from typing import Dict, Optional, cast +from typing import ( + Collection, + Dict, + Iterable, + List, + Optional, + Union, + cast, +) from .common import c_name from .error import QAPISemError @@ -37,12 +45,14 @@ r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) -def check_name_is_str(name, info, source): +def check_name_is_str(name: object, + info: QAPISourceInfo, + source: str) -> None: if not isinstance(name, str): raise QAPISemError(info, "%s requires a string name" % source) -def check_name_str(name, info, source): +def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' # and 'q_obj_*' implicit type names. match = valid_name.match(name) @@ -51,16 +61,16 @@ def check_name_str(name, info, source): return match.group(3) -def check_name_upper(name, info, source): +def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: stem = check_name_str(name, info, source) if re.search(r'[a-z-]', stem): raise QAPISemError( info, "name of %s must not use lowercase or '-'" % source) -def check_name_lower(name, info, source, - permit_upper=False, - permit_underscore=False): +def check_name_lower(name: str, info: QAPISourceInfo, source: str, + permit_upper: bool = False, + permit_underscore: bool = False) -> None: stem = check_name_str(name, info, source) if ((not permit_upper and re.search(r'[A-Z]', stem)) or (not permit_underscore and '_' in stem)): @@ -68,13 +78,13 @@ def check_name_lower(name, info, source, info, "name of %s must not use uppercase or '_'" % source) -def check_name_camel(name, info, source): +def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None: stem = check_name_str(name, info, source) if not re.match(r'[A-Z][A-Za-z0-9]*[a-z][A-Za-z0-9]*$', stem): raise QAPISemError(info, "name of %s must use CamelCase" % source) -def check_defn_name_str(name, info, meta): +def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None: if meta == 'event': check_name_upper(name, info, meta) elif meta == 'command': @@ -88,9 +98,13 @@ def check_defn_name_str(name, info, meta): info, "%s name should not end in '%s'" % (meta, name[-4:])) -def check_keys(value, info, source, required, optional): +def check_keys(value: _JSONObject, + info: QAPISourceInfo, + source: str, + required: Collection[str], + optional: Collection[str]) -> None: - def pprint(elems): + def pprint(elems: Iterable[str]) -> str: return ', '.join("'" + e + "'" for e in sorted(elems)) missing = set(required) - set(value) @@ -110,7 +124,7 @@ def pprint(elems): pprint(unknown), pprint(allowed))) -def check_flags(expr, info): +def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: for key in ['gen', 'success-response']: if key in expr and expr[key] is not False: raise QAPISemError( @@ -128,9 +142,9 @@ def check_flags(expr, info): "are incompatible") -def check_if(expr, info, source): +def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: - def check_if_str(ifcond): + def check_if_str(ifcond: object) -> None: if not isinstance(ifcond, str): raise QAPISemError( info, @@ -156,7 +170,7 @@ def check_if_str(ifcond): expr['if'] = [ifcond] -def normalize_members(members): +def normalize_members(members: object) -> None: if isinstance(members, dict): for key, arg in members.items(): if isinstance(arg, dict): @@ -164,8 +178,11 @@ def normalize_members(members): members[key] = {'type': arg} -def check_type(value, info, source, - allow_array=False, allow_dict=False): +def check_type(value: Optional[object], + info: QAPISourceInfo, + source: str, + allow_array: bool = False, + allow_dict: Union[bool, str] = False) -> None: if value is None: return @@ -212,7 +229,8 @@ def check_type(value, info, source, check_type(arg['type'], info, key_source, allow_array=True) -def check_features(features, info): +def check_features(features: Optional[object], + info: QAPISourceInfo) -> None: if features is None: return if not isinstance(features, list): @@ -229,7 +247,7 @@ def check_features(features, info): check_if(f, info, source) -def check_enum(expr, info): +def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: name = expr['enum'] members = expr['data'] prefix = expr.get('prefix') @@ -258,7 +276,7 @@ def check_enum(expr, info): check_if(member, info, source) -def check_struct(expr, info): +def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: name = cast(str, expr['struct']) # Asserted in check_exprs members = expr['data'] @@ -266,7 +284,7 @@ def check_struct(expr, info): check_type(expr.get('base'), info, "'base'") -def check_union(expr, info): +def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: name = cast(str, expr['union']) # Asserted in check_exprs base = expr.get('base') discriminator = expr.get('discriminator') @@ -294,7 +312,7 @@ def check_union(expr, info): check_type(value['type'], info, source, allow_array=not base) -def check_alternate(expr, info): +def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: members = expr['data'] if not members: @@ -311,7 +329,7 @@ def check_alternate(expr, info): check_type(value['type'], info, source) -def check_command(expr, info): +def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: args = expr.get('data') rets = expr.get('returns') boxed = expr.get('boxed', False) @@ -322,7 +340,7 @@ def check_command(expr, info): check_type(rets, info, "'returns'", allow_array=True) -def check_event(expr, info): +def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: args = expr.get('data') boxed = expr.get('boxed', False) @@ -331,7 +349,7 @@ def check_event(expr, info): check_type(args, info, "'data'", allow_dict=not boxed) -def check_exprs(exprs): +def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: for expr_elem in exprs: # Expression assert isinstance(expr_elem['expr'], dict) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 0a000d58b3..7797c83432 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -8,11 +8,6 @@ disallow_untyped_defs = False disallow_incomplete_defs = False check_untyped_defs = False -[mypy-qapi.expr] -disallow_untyped_defs = False -disallow_incomplete_defs = False -check_untyped_defs = False - [mypy-qapi.parser] disallow_untyped_defs = False disallow_incomplete_defs = False -- 2.30.2
This is a small rewrite to address some minor style nits. Don't compare against the empty list to check for the empty condition, and move the normalization forward to unify the check on the now-normalized structure. With the check unified, the local nested function isn't needed anymore and can be brought down into the normal flow of the function. With the nesting level changed, shuffle the error strings around a bit to get them to fit in 79 columns. Note: although ifcond is typed as Sequence[str] elsewhere, we *know* that the parser will produce real, bona-fide lists. It's okay to check isinstance(ifcond, list) here. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index ea9d39fcf2..5921fa34ab 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -144,30 +144,26 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: - def check_if_str(ifcond: object) -> None: - if not isinstance(ifcond, str): - raise QAPISemError( - info, - "'if' condition of %s must be a string or a list of strings" - % source) - if ifcond.strip() == '': - raise QAPISemError( - info, - "'if' condition '%s' of %s makes no sense" - % (ifcond, source)) - ifcond = expr.get('if') if ifcond is None: return + if isinstance(ifcond, list): - if ifcond == []: + if not ifcond: raise QAPISemError( - info, "'if' condition [] of %s is useless" % source) - for elt in ifcond: - check_if_str(elt) + info, f"'if' condition [] of {source} is useless") else: - check_if_str(ifcond) - expr['if'] = [ifcond] + # Normalize to a list + ifcond = expr['if'] = [ifcond] + + for elt in ifcond: + if not isinstance(elt, str): + raise QAPISemError(info, ( + f"'if' condition of {source}" + " must be a string or a list of strings")) + if not elt.strip(): + raise QAPISemError( + info, f"'if' condition '{elt}' of {source} makes no sense") def normalize_members(members: object) -> None: -- 2.30.2
Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 5921fa34ab..1869ddf815 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -233,14 +233,14 @@ def check_features(features: Optional[object], raise QAPISemError(info, "'features' must be an array") features[:] = [f if isinstance(f, dict) else {'name': f} for f in features] - for f in features: + for feat in features: source = "'features' member" - assert isinstance(f, dict) - check_keys(f, info, source, ['name'], ['if']) - check_name_is_str(f['name'], info, source) - source = "%s '%s'" % (source, f['name']) - check_name_lower(f['name'], info, source) - check_if(f, info, source) + assert isinstance(feat, dict) + check_keys(feat, info, source, ['name'], ['if']) + check_name_is_str(feat['name'], info, source) + source = "%s '%s'" % (source, feat['name']) + check_name_str(feat['name'], info, source) + check_if(feat, info, source) def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: -- 2.30.2
Signed-off-by: John Snow <jsnow@redhat.com> Tested-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Cleber Rosa <crosa@redhat.com> Tested-by: Cleber Rosa <crosa@redhat.com> --- scripts/qapi/pylintrc | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc index b9e077a164..fb0386d529 100644 --- a/scripts/qapi/pylintrc +++ b/scripts/qapi/pylintrc @@ -3,7 +3,6 @@ # Add files or directories matching the regex patterns to the ignore list. # The regex matches against base names, not paths. ignore-patterns=error.py, - expr.py, parser.py, schema.py, -- 2.30.2
Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 213 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 208 insertions(+), 5 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 1869ddf815..adc5b903bc 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -1,7 +1,5 @@ # -*- coding: utf-8 -*- # -# Check (context-free) QAPI schema expression structure -# # Copyright IBM, Corp. 2011 # Copyright (c) 2013-2019 Red Hat Inc. # @@ -14,6 +12,25 @@ # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. +""" +Normalize and validate (context-free) QAPI schema expression structures. + +After QAPI expressions are parsed from disk, they are stored in +recursively nested Python data structures using Dict, List, str, bool, +and int. This module ensures that those nested structures have the +correct type(s) and key(s) where appropriate for the QAPI context-free +grammar. + +The QAPI schema expression language allows for syntactic sugar; this +module also handles the normalization process of these nested +structures. + +See `check_exprs` for the main entry point. + +See `schema.QAPISchema` for processing into native Python data +structures and contextual semantic validation. +""" + import re from typing import ( Collection, @@ -31,9 +48,10 @@ from .source import QAPISourceInfo -# Deserialized JSON objects as returned by the parser; -# The values of this mapping are not necessary to exhaustively type -# here, because the purpose of this module is to interrogate that type. +#: Deserialized JSON objects as returned by the parser. +#: +#: The values of this mapping are not necessary to exhaustively type +#: here, because the purpose of this module is to interrogate that type. _JSONObject = Dict[str, object] @@ -48,11 +66,29 @@ def check_name_is_str(name: object, info: QAPISourceInfo, source: str) -> None: + """Ensures that ``name`` is a string.""" if not isinstance(name, str): raise QAPISemError(info, "%s requires a string name" % source) def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: + """ + Ensures a string is a legal name. + + A legal name consists of ascii letters, digits, ``-``, and ``_``, + starting with a letter. The names of downstream extensions are + prefixed with an __com.example_ style prefix, allowing ``.`` and + ``-``. An experimental name is prefixed with ``x-``, following the + RFQDN if present. + + A legal name cannot start with ``q_``, which is reserved. + + :param name: Name to check. + :param info: QAPI source file information. + :param source: Human-readable str describing "what" this name is. + + :return: The stem of the valid name, with no prefixes. + """ # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' # and 'q_obj_*' implicit type names. match = valid_name.match(name) @@ -62,6 +98,12 @@ def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: + """ + Ensures a string is a legal event name. + + Checks the same criteria as `check_name_str`, but requires uppercase + and prohibits ``-``. + """ stem = check_name_str(name, info, source) if re.search(r'[a-z-]', stem): raise QAPISemError( @@ -71,6 +113,15 @@ def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: def check_name_lower(name: str, info: QAPISourceInfo, source: str, permit_upper: bool = False, permit_underscore: bool = False) -> None: + """ + Ensures a string is a legal user defined type name. + + Checks the same criteria as `check_name_str`, but may impose + additional constraints. + + :param permit_upper: Prohibits uppercase when false. + :param permit_underscore: Prohibits underscores when false. + """ stem = check_name_str(name, info, source) if ((not permit_upper and re.search(r'[A-Z]', stem)) or (not permit_underscore and '_' in stem)): @@ -79,12 +130,31 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None: + """ + Ensures a string is a legal CamelCase name. + + Checks the same criteria as `check_name_str`, + but additionally imposes a CamelCase constraint. + """ stem = check_name_str(name, info, source) if not re.match(r'[A-Z][A-Za-z0-9]*[a-z][A-Za-z0-9]*$', stem): raise QAPISemError(info, "name of %s must use CamelCase" % source) def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None: + """ + Ensures a name is a legal definition name. + + - 'event' names adhere to `check_name_upper`. + - 'command' names adhere to `check_name_lower`. + - All other names adhere to `check_name_camel`. + + All name types must not end with ``Kind`` nor ``List``. + + :param name: Name to check. + :param info: QAPI source file information. + :param meta: Type name of the QAPI expression. + """ if meta == 'event': check_name_upper(name, info, meta) elif meta == 'command': @@ -103,6 +173,15 @@ def check_keys(value: _JSONObject, source: str, required: Collection[str], optional: Collection[str]) -> None: + """ + Ensures an object has a specific set of keys. + + :param value: The object to check. + :param info: QAPI source file information. + :param source: Human-readable str describing this value. + :param required: Keys that *must* be present. + :param optional: Keys that *may* be present. + """ def pprint(elems: Iterable[str]) -> str: return ', '.join("'" + e + "'" for e in sorted(elems)) @@ -125,6 +204,12 @@ def pprint(elems: Iterable[str]) -> str: def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: + """ + Ensures common fields in an expression are correct. + + :param expr: Expression to validate. + :param info: QAPI source file information. + """ for key in ['gen', 'success-response']: if key in expr and expr[key] is not False: raise QAPISemError( @@ -143,7 +228,22 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: + """ + Syntactically validate and normalize the ``if`` field of an object. + The ``if`` field may be either a ``str`` or a ``List[str]``. + A ``str`` element will be normalized to ``List[str]``. + + :forms: + :sugared: ``Union[str, List[str]]`` + :canonical: ``List[str]`` + + :param expr: A ``dict``. + The ``if`` field, if present, will be validated. + :param info: QAPI source file information. + + :return: None, ``expr`` is normalized in-place as needed. + """ ifcond = expr.get('if') if ifcond is None: return @@ -167,6 +267,20 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: def normalize_members(members: object) -> None: + """ + Normalize a "members" value. + + If ``members`` is an object, for every value in that object, if that + value is not itself already an object, normalize it to + ``{'type': value}``. + + :forms: + :sugared: ``Dict[str, Union[str, TypeRef]]`` + :canonical: ``Dict[str, TypeRef]`` + + :param members: The members object to normalize. + :return: None, ``members`` is normalized in-place as needed. + """ if isinstance(members, dict): for key, arg in members.items(): if isinstance(arg, dict): @@ -179,6 +293,23 @@ def check_type(value: Optional[object], source: str, allow_array: bool = False, allow_dict: Union[bool, str] = False) -> None: + """ + Check the QAPI type of ``value``. + + Python types of ``str`` or ``None`` are always allowed. + + :param value: The value to check. + :param info: QAPI Source file information. + :param source: Human-readable str describing this value. + :param allow_array: Allow a ``List[str]`` of length 1, + which indicates an Array<T> type. + :param allow_dict: Allow a dict, treated as an anonymous type. + When given a string, check if that name is + allowed to have keys that use uppercase letters, + and modify validation accordingly. + + :return: None, ``value`` is normalized in-place as needed. + """ if value is None: return @@ -227,6 +358,21 @@ def check_type(value: Optional[object], def check_features(features: Optional[object], info: QAPISourceInfo) -> None: + """ + Syntactically validate and normalize the ``features`` field. + + ``features`` may be a ``list`` of either ``str`` or ``dict``. + Any ``str`` element will be normalized to ``{'name': element}``. + + :forms: + :sugared: ``List[Union[str, Feature]]`` + :canonical: ``List[Feature]`` + + :param features: an optional list of either str or dict. + :param info: QAPI Source file information. + + :return: None, ``features`` is normalized in-place as needed. + """ if features is None: return if not isinstance(features, list): @@ -244,6 +390,14 @@ def check_features(features: Optional[object], def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: + """ + Validate this expression as an ``enum`` definition. + + :param expr: The expression to validate. + :param info: QAPI source file information. + + :return: None, ``expr`` is normalized in-place as needed. + """ name = expr['enum'] members = expr['data'] prefix = expr.get('prefix') @@ -273,6 +427,14 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: + """ + Validate this expression as a ``struct`` definition. + + :param expr: The expression to validate. + :param info: QAPI source file information. + + :return: None, ``expr`` is normalized in-place as needed. + """ name = cast(str, expr['struct']) # Asserted in check_exprs members = expr['data'] @@ -281,6 +443,14 @@ def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: + """ + Validate this expression as a ``union`` definition. + + :param expr: The expression to validate. + :param info: QAPI source file information. + + :return: None, ``expr`` is normalized in-place as needed. + """ name = cast(str, expr['union']) # Asserted in check_exprs base = expr.get('base') discriminator = expr.get('discriminator') @@ -309,6 +479,14 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: + """ + Validate this expression as an ``alternate`` definition. + + :param expr: The expression to validate. + :param info: QAPI source file information. + + :return: None, ``expr`` is normalized in-place as needed. + """ members = expr['data'] if not members: @@ -326,6 +504,14 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: + """ + Validate this expression as a ``command`` definition. + + :param expr: The expression to validate. + :param info: QAPI source file information. + + :return: None, ``expr`` is normalized in-place as needed. + """ args = expr.get('data') rets = expr.get('returns') boxed = expr.get('boxed', False) @@ -337,6 +523,14 @@ def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: + """ + Normalize and validate this expression as an ``event`` definition. + + :param expr: The expression to validate. + :param info: QAPI source file information. + + :return: None, ``expr`` is normalized in-place as needed. + """ args = expr.get('data') boxed = expr.get('boxed', False) @@ -346,6 +540,15 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: + """ + Validate and normalize a list of parsed QAPI schema expressions. + + This function accepts a list of expressions + metadta as returned by + the parser. It destructively normalizes the expressions in-place. + + :param exprs: The list of expressions to normalize/validate. + :return: The same list of expressions (now modified). + """ for expr_elem in exprs: # Expression assert isinstance(expr_elem['expr'], dict) -- 2.30.2
It is -- maybe -- possibly -- three nanoseconds faster. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Cleber Rosa <crosa@redhat.com> --- This can be dropped if desired; it has no real functional impact I could defend in code review court. I just happened to write it this way. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index adc5b903bc..b11c11b965 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -210,11 +210,11 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: :param expr: Expression to validate. :param info: QAPI source file information. """ - for key in ['gen', 'success-response']: + for key in ('gen', 'success-response'): if key in expr and expr[key] is not False: raise QAPISemError( info, "flag '%s' may only use false value" % key) - for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']: + for key in ('boxed', 'allow-oob', 'allow-preconfig', 'coroutine'): if key in expr and expr[key] is not True: raise QAPISemError( info, "flag '%s' may only use true value" % key) -- 2.30.2
There's not a big obvious difference between the types of checks that happen in the main function versus the kind that happen in the functions. Now they're in one place for each of the main types. As part of the move, spell out the required and optional keywords so they're obvious at a glance. Use tuples instead of lists for immutable data, too. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Cleber Rosa <crosa@redhat.com> --- scripts/qapi/expr.py | 55 ++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index b11c11b965..aabbc255d2 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -398,6 +398,10 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: :return: None, ``expr`` is normalized in-place as needed. """ + check_keys(expr, info, 'enum', + required=('enum', 'data'), + optional=('if', 'features', 'prefix')) + name = expr['enum'] members = expr['data'] prefix = expr.get('prefix') @@ -435,6 +439,11 @@ def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: :return: None, ``expr`` is normalized in-place as needed. """ + check_keys(expr, info, 'struct', + required=('struct', 'data'), + optional=('base', 'if', 'features')) + normalize_members(expr['data']) + name = cast(str, expr['struct']) # Asserted in check_exprs members = expr['data'] @@ -451,6 +460,13 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: :return: None, ``expr`` is normalized in-place as needed. """ + check_keys(expr, info, 'union', + required=('union', 'data'), + optional=('base', 'discriminator', 'if', 'features')) + + normalize_members(expr.get('base')) + normalize_members(expr['data']) + name = cast(str, expr['union']) # Asserted in check_exprs base = expr.get('base') discriminator = expr.get('discriminator') @@ -487,6 +503,11 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: :return: None, ``expr`` is normalized in-place as needed. """ + check_keys(expr, info, 'alternate', + required=('alternate', 'data'), + optional=('if', 'features')) + normalize_members(expr['data']) + members = expr['data'] if not members: @@ -512,6 +533,13 @@ def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: :return: None, ``expr`` is normalized in-place as needed. """ + check_keys(expr, info, 'command', + required=('command',), + optional=('data', 'returns', 'boxed', 'if', 'features', + 'gen', 'success-response', 'allow-oob', + 'allow-preconfig', 'coroutine')) + normalize_members(expr.get('data')) + args = expr.get('data') rets = expr.get('returns') boxed = expr.get('boxed', False) @@ -531,6 +559,11 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: :return: None, ``expr`` is normalized in-place as needed. """ + check_keys(expr, info, 'event', + required=('event',), + optional=('data', 'boxed', 'if', 'features')) + normalize_members(expr.get('data')) + args = expr.get('data') boxed = expr.get('boxed', False) @@ -598,38 +631,16 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: "documentation comment required") if meta == 'enum': - check_keys(expr, info, meta, - ['enum', 'data'], ['if', 'features', 'prefix']) check_enum(expr, info) elif meta == 'union': - check_keys(expr, info, meta, - ['union', 'data'], - ['base', 'discriminator', 'if', 'features']) - normalize_members(expr.get('base')) - normalize_members(expr['data']) check_union(expr, info) elif meta == 'alternate': - check_keys(expr, info, meta, - ['alternate', 'data'], ['if', 'features']) - normalize_members(expr['data']) check_alternate(expr, info) elif meta == 'struct': - check_keys(expr, info, meta, - ['struct', 'data'], ['base', 'if', 'features']) - normalize_members(expr['data']) check_struct(expr, info) elif meta == 'command': - check_keys(expr, info, meta, - ['command'], - ['data', 'returns', 'boxed', 'if', 'features', - 'gen', 'success-response', 'allow-oob', - 'allow-preconfig', 'coroutine']) - normalize_members(expr.get('data')) check_command(expr, info) elif meta == 'event': - check_keys(expr, info, meta, - ['event'], ['data', 'boxed', 'if', 'features']) - normalize_members(expr.get('data')) check_event(expr, info) else: assert False, 'unexpected meta type' -- 2.30.2
This enforces a type signature against all of the top-level expression check routines without necessarily needing to create some overcomplicated class hierarchy for them. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 63 +++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index aabbc255d2..c42d061e68 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -31,8 +31,10 @@ structures and contextual semantic validation. """ +from enum import Enum import re from typing import ( + Callable, Collection, Dict, Iterable, @@ -572,6 +574,29 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: check_type(args, info, "'data'", allow_dict=not boxed) +class ExpressionType(str, Enum): + INCLUDE = 'include' + ENUM = 'enum' + UNION = 'union' + ALTERNATE = 'alternate' + STRUCT = 'struct' + COMMAND = 'command' + EVENT = 'event' + + def __str__(self) -> str: + return str(self.value) + + +_CHECK_FN: Dict[str, Callable[[_JSONObject, QAPISourceInfo], None]] = { + 'enum': check_enum, + 'union': check_union, + 'alternate': check_alternate, + 'struct': check_struct, + 'command': check_command, + 'event': check_event, +} + + def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: """ Validate and normalize a list of parsed QAPI schema expressions. @@ -598,24 +623,16 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: assert tmp is None or isinstance(tmp, QAPIDoc) doc: Optional[QAPIDoc] = tmp - if 'include' in expr: - continue - - if 'enum' in expr: - meta = 'enum' - elif 'union' in expr: - meta = 'union' - elif 'alternate' in expr: - meta = 'alternate' - elif 'struct' in expr: - meta = 'struct' - elif 'command' in expr: - meta = 'command' - elif 'event' in expr: - meta = 'event' + for kind in ExpressionType: + if kind in expr: + meta = kind + break else: raise QAPISemError(info, "expression is missing metatype") + if meta == ExpressionType.INCLUDE: + continue + check_name_is_str(expr[meta], info, "'%s'" % meta) name = cast(str, expr[meta]) info.set_defn(meta, name) @@ -630,21 +647,7 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: raise QAPISemError(info, "documentation comment required") - if meta == 'enum': - check_enum(expr, info) - elif meta == 'union': - check_union(expr, info) - elif meta == 'alternate': - check_alternate(expr, info) - elif meta == 'struct': - check_struct(expr, info) - elif meta == 'command': - check_command(expr, info) - elif meta == 'event': - check_event(expr, info) - else: - assert False, 'unexpected meta type' - + _CHECK_FN[meta](expr, info) check_if(expr, info, meta) check_features(expr.get('features'), info) check_flags(expr, info) -- 2.30.2
John Snow <jsnow@redhat.com> writes: > mypy does not know the types of values stored in Dicts that masquerade > as objects. Help the type checker out by constraining the type. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index b4bbcd54c0..b75c85c160 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -15,9 +15,18 @@ > # See the COPYING file in the top-level directory. > > import re > +from typing import Dict, Optional > > from .common import c_name > from .error import QAPISemError > +from .parser import QAPIDoc > +from .source import QAPISourceInfo > + > + > +# Deserialized JSON objects as returned by the parser; > +# The values of this mapping are not necessary to exhaustively type Not necessary and also not practical with current mypy. Correct? > +# here, because the purpose of this module is to interrogate that type. > +_JSONObject = Dict[str, object] > > > # Names consist of letters, digits, -, and _, starting with a letter. > @@ -315,9 +324,20 @@ def check_event(expr, info): > > def check_exprs(exprs): > for expr_elem in exprs: > - expr = expr_elem['expr'] > - info = expr_elem['info'] > - doc = expr_elem.get('doc') > + # Expression > + assert isinstance(expr_elem['expr'], dict) I dislike relaxing OrderedDict to dict here. I'm going to accept it anyway, because the difference between the two is going away in 3.7, and because so far order actually matters only in certain sub-expressions, not top-level expressions. > + for key in expr_elem['expr'].keys(): > + assert isinstance(key, str) > + expr: _JSONObject = expr_elem['expr'] > + > + # QAPISourceInfo > + assert isinstance(expr_elem['info'], QAPISourceInfo) > + info: QAPISourceInfo = expr_elem['info'] > + > + # Optional[QAPIDoc] > + tmp = expr_elem.get('doc') > + assert tmp is None or isinstance(tmp, QAPIDoc) > + doc: Optional[QAPIDoc] = tmp > > if 'include' in expr: > continue I see you didn't bite on the idea to do less checking here. Okay.
John Snow <jsnow@redhat.com> writes: > It needs to be an object (dict), not anything else. > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > Note: this actually doesn't ... work, but on-list, we discussed wanting > tests first, then the fix. That can't happen here, because QAPI crashes > at runtime. So uh, just squash this into the following patch, I guess? Yes. > I tried. Thanks! > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qapi-schema/alternate-invalid-data-type.err | 0 > tests/qapi-schema/alternate-invalid-data-type.json | 4 ++++ > tests/qapi-schema/alternate-invalid-data-type.out | 0 > tests/qapi-schema/meson.build | 2 ++ > tests/qapi-schema/union-invalid-data-type.err | 0 > tests/qapi-schema/union-invalid-data-type.json | 13 +++++++++++++ > tests/qapi-schema/union-invalid-data-type.out | 0 > 7 files changed, 19 insertions(+) > 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 > > diff --git a/tests/qapi-schema/alternate-invalid-data-type.err b/tests/qapi-schema/alternate-invalid-data-type.err > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qapi-schema/alternate-invalid-data-type.json b/tests/qapi-schema/alternate-invalid-data-type.json > new file mode 100644 > index 0000000000..7d5d905581 > --- /dev/null > +++ b/tests/qapi-schema/alternate-invalid-data-type.json > @@ -0,0 +1,4 @@ > +# Alternate type requires an object for 'data' > +{ 'alternate': 'Alt', > + 'data': ['rubbish', 'nonsense'] > +} Let's name it alternate-data-invalid.json, for consistency with struct-data-invalid.json > diff --git a/tests/qapi-schema/alternate-invalid-data-type.out b/tests/qapi-schema/alternate-invalid-data-type.out > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build > index 8ba6917132..cc5b136cfb 100644 > --- a/tests/qapi-schema/meson.build > +++ b/tests/qapi-schema/meson.build > @@ -15,6 +15,7 @@ schemas = [ > 'alternate-conflict-bool-string.json', > 'alternate-conflict-num-string.json', > 'alternate-empty.json', > + 'alternate-invalid-data-type.json', > 'alternate-invalid-dict.json', > 'alternate-nested.json', > 'alternate-unknown.json', > @@ -192,6 +193,7 @@ schemas = [ > 'union-clash-branches.json', > 'union-empty.json', > 'union-invalid-base.json', > + 'union-invalid-data-type.json', > 'union-optional-branch.json', > 'union-unknown.json', > 'unknown-escape.json', > diff --git a/tests/qapi-schema/union-invalid-data-type.err b/tests/qapi-schema/union-invalid-data-type.err > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qapi-schema/union-invalid-data-type.json b/tests/qapi-schema/union-invalid-data-type.json > new file mode 100644 > index 0000000000..5a32d267bf > --- /dev/null > +++ b/tests/qapi-schema/union-invalid-data-type.json > @@ -0,0 +1,13 @@ > +# the union data type must be an object. > + > +{ 'struct': 'TestTypeA', > + 'data': { 'string': 'str' } } > + > +{ 'struct': 'TestTypeB', > + 'data': { 'integer': 'int' } } These two seem superfluous. > + > +{ 'union': 'TestUnion', > + 'base': 'int', > + 'discriminator': 'int', > + 'data': ['TestTypeA', 'TestTypeB'] > +} Name it union-invalid-data.json. > diff --git a/tests/qapi-schema/union-invalid-data-type.out b/tests/qapi-schema/union-invalid-data-type.out > new file mode 100644 > index 0000000000..e69de29bb2
Suggest qapi/expr.py: Check type of union and alternate 'data' member John Snow <jsnow@redhat.com> writes: > We don't actually check, so the user can get some unpleasant stacktraces. Let's point to the new tests here. > Formalize it. Huh? > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 7 +++++++ > tests/qapi-schema/alternate-invalid-data-type.err | 2 ++ > tests/qapi-schema/union-invalid-data-type.err | 2 ++ > 3 files changed, 11 insertions(+) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 73e7d8cb0d..ca5ab7bfda 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -281,6 +281,9 @@ def check_union(expr, info): > raise QAPISemError(info, "'discriminator' requires 'base'") > check_name_is_str(discriminator, info, "'discriminator'") > > + if not isinstance(members, dict): > + raise QAPISemError(info, "'data' must be an object") > + > for (key, value) in members.items(): > source = "'data' member '%s'" % key > if discriminator is None: > @@ -296,6 +299,10 @@ def check_alternate(expr, info): > > if not members: > raise QAPISemError(info, "'data' must not be empty") > + > + if not isinstance(members, dict): > + raise QAPISemError(info, "'data' must be an object") > + > for (key, value) in members.items(): > source = "'data' member '%s'" % key > check_name_lower(key, info, source) > diff --git a/tests/qapi-schema/alternate-invalid-data-type.err b/tests/qapi-schema/alternate-invalid-data-type.err > index e69de29bb2..c7301ccb00 100644 > --- a/tests/qapi-schema/alternate-invalid-data-type.err > +++ b/tests/qapi-schema/alternate-invalid-data-type.err > @@ -0,0 +1,2 @@ > +alternate-invalid-data-type.json: In alternate 'Alt': > +alternate-invalid-data-type.json:2: 'data' must be an object > diff --git a/tests/qapi-schema/union-invalid-data-type.err b/tests/qapi-schema/union-invalid-data-type.err > index e69de29bb2..b71c3400c5 100644 > --- a/tests/qapi-schema/union-invalid-data-type.err > +++ b/tests/qapi-schema/union-invalid-data-type.err > @@ -0,0 +1,2 @@ > +union-invalid-data-type.json: In union 'TestUnion': > +union-invalid-data-type.json:9: 'data' must be an object
John Snow <jsnow@redhat.com> writes: > Casts are instructions to the type checker only, they aren't "safe" and > should probably be avoided in general. In this case, when we perform > type checking on a nested structure, the type of each field does not > "stick". > > (See PEP 647 for an example of "type narrowing" that does "stick". > It is available in Python 3.10, so we can't use it yet.) > > We don't need to assert that something is a str if we've already checked > or asserted that it is -- use a cast instead for these cases. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index ca5ab7bfda..505e67bd21 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -15,7 +15,7 @@ > # See the COPYING file in the top-level directory. > > import re > -from typing import Dict, Optional > +from typing import Dict, Optional, cast > > from .common import c_name > from .error import QAPISemError > @@ -259,7 +259,7 @@ def check_enum(expr, info): > > > def check_struct(expr, info): > - name = expr['struct'] > + name = cast(str, expr['struct']) # Asserted in check_exprs "Asserted" suggests an an assert statement. It's actually the check_name_is_str() visible in the last hunk. What about "Checked in check_exprs()" or "Ensured by check_exprs()"? > members = expr['data'] > > check_type(members, info, "'data'", allow_dict=name) > @@ -267,7 +267,7 @@ def check_struct(expr, info): > > > def check_union(expr, info): > - name = expr['union'] > + name = cast(str, expr['union']) # Asserted in check_exprs > base = expr.get('base') > discriminator = expr.get('discriminator') > members = expr['data'] > @@ -366,8 +366,8 @@ def check_exprs(exprs): > else: > raise QAPISemError(info, "expression is missing metatype") > > - name = expr[meta] > - check_name_is_str(name, info, "'%s'" % meta) > + check_name_is_str(expr[meta], info, "'%s'" % meta) > + name = cast(str, expr[meta]) > info.set_defn(meta, name) > check_defn_name_str(name, info, meta)
John Snow <jsnow@redhat.com> writes: > This is a minor adjustment that allows the 'required' and 'optional' > keys fields to take a default value of an empty, immutable sequence (the > empty tuple). > > This reveals a quirk of this function, which is that "a + b" is > list-specific behavior. We can accept a wider variety of types if we > avoid that behavior. Using Collection allows us to accept things like > lists, tuples, sets, and so on. > > (Iterable would also have worked, but Iterable also includes things like > generator expressions which are consumed upon iteration, which would > require a rewrite to make sure that each input was only traversed once.) > > Signed-off-by: John Snow <jsnow@redhat.com> The commit message confused me briefly, until I realized v3 of this patch came later in the series, where it modified check_keys() type hints and added default values. What about this: This is a minor adjustment that lets parameters @required and @optional take tuple arguments, in particular (). Later patches will make use of that. > --- > scripts/qapi/expr.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 505e67bd21..7e22723b50 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -100,7 +100,7 @@ def pprint(elems): > "%s misses key%s %s" > % (source, 's' if len(missing) > 1 else '', > pprint(missing))) > - allowed = set(required + optional) > + allowed = set(required) | set(optional) > unknown = set(value) - allowed > if unknown: > raise QAPISemError(
John Snow <jsnow@redhat.com> writes: > This is a small rewrite to address some minor style nits. > > Don't compare against the empty list to check for the empty condition, and > move the normalization forward to unify the check on the now-normalized > structure. > > With the check unified, the local nested function isn't needed anymore > and can be brought down into the normal flow of the function. With the > nesting level changed, shuffle the error strings around a bit to get > them to fit in 79 columns. > > Note: although ifcond is typed as Sequence[str] elsewhere, we *know* that > the parser will produce real, bona-fide lists. It's okay to check > isinstance(ifcond, list) here. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index ea9d39fcf2..5921fa34ab 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -144,30 +144,26 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: > > def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: > > - def check_if_str(ifcond: object) -> None: > - if not isinstance(ifcond, str): > - raise QAPISemError( > - info, > - "'if' condition of %s must be a string or a list of strings" > - % source) > - if ifcond.strip() == '': > - raise QAPISemError( > - info, > - "'if' condition '%s' of %s makes no sense" > - % (ifcond, source)) > - > ifcond = expr.get('if') > if ifcond is None: > return > + > if isinstance(ifcond, list): > - if ifcond == []: > + if not ifcond: > raise QAPISemError( > - info, "'if' condition [] of %s is useless" % source) > - for elt in ifcond: > - check_if_str(elt) > + info, f"'if' condition [] of {source} is useless") Unrelated change from interpolation to formatted string literal. > else: > - check_if_str(ifcond) > - expr['if'] = [ifcond] > + # Normalize to a list > + ifcond = expr['if'] = [ifcond] > + > + for elt in ifcond: > + if not isinstance(elt, str): > + raise QAPISemError(info, ( > + f"'if' condition of {source}" > + " must be a string or a list of strings")) > + if not elt.strip(): > + raise QAPISemError( > + info, f"'if' condition '{elt}' of {source} makes no sense") Likewise. I like formatted string literals, they're often easier to read than interpolation. But let's try to keep patches focused on their stated purpose. I'd gladly consider a series that convers to formatted strings wholesale. But I guess we better finish the typing job, first. > > > def normalize_members(members: object) -> None:
John Snow <jsnow@redhat.com> writes:
> It is -- maybe -- possibly -- three nanoseconds faster.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>
> ---
>
> This can be dropped if desired; it has no real functional impact I could
> defend in code review court. I just happened to write it this way.
I'm fine with taking this. Could it go right after PATCH 11? If not,
no big deal.
John Snow <jsnow@redhat.com> writes:
> PEP8's BDFL writes: "For flowing long blocks of text with fewer
> structural restrictions (docstrings or comments), the line length should
> be limited to 72 characters."
>
> I do not like this patch. I have included it explicitly to recommend we
> do not pay any further heed to the 72 column limit.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
I'd like to get the remainder of this series moving again before digging
into this patch.
John Snow <jsnow@redhat.com> writes: > Fixes: 0825f62c842 For a slightly peculiar value of "fixes" ;) > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 540b3982b1..c207481f7e 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -241,7 +241,7 @@ def check_enum(expr, info): > source = "%s '%s'" % (source, member_name) > # Enum members may start with a digit > if member_name[0].isdigit(): > - member_name = 'd' + member_name # Hack: hide the digit > + member_name = 'd' + member_name # Hack: hide the digit > check_name_lower(member_name, info, source, > permit_upper=permissive, > permit_underscore=permissive)
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 11:41 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Fixes: 0825f62c842
>
> For a slightly peculiar value of "fixes" ;)
>
Less of this, please.
On 3/25/21 11:21 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> PEP8's BDFL writes: "For flowing long blocks of text with fewer
>> structural restrictions (docstrings or comments), the line length should
>> be limited to 72 characters."
>>
>> I do not like this patch. I have included it explicitly to recommend we
>> do not pay any further heed to the 72 column limit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> I'd like to get the remainder of this series moving again before digging
> into this patch.
>
I am dropping it, then -- I have no interest in bringing a patch I
dislike along for another respin.
--js
On 3/25/21 10:04 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> mypy does not know the types of values stored in Dicts that masquerade >> as objects. Help the type checker out by constraining the type. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/expr.py | 26 +++++++++++++++++++++++--- >> 1 file changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >> index b4bbcd54c0..b75c85c160 100644 >> --- a/scripts/qapi/expr.py >> +++ b/scripts/qapi/expr.py >> @@ -15,9 +15,18 @@ >> # See the COPYING file in the top-level directory. >> >> import re >> +from typing import Dict, Optional >> >> from .common import c_name >> from .error import QAPISemError >> +from .parser import QAPIDoc >> +from .source import QAPISourceInfo >> + >> + >> +# Deserialized JSON objects as returned by the parser; >> +# The values of this mapping are not necessary to exhaustively type > > Not necessary and also not practical with current mypy. Correct? > Neither necessary nor practical. Typing as 'object' guarantees that these values will never be used in a manner not supported by all python objects. Mypy does not complain, so we know that we don't misuse the type. This is helpful for proving the validity of the expr.py validator itself: we know that we are not forgetting to perform type narrowing and using the value contained therein inappropriately. Adding a more exhaustive typing here is impractical (for reasons we learned during introspect.py), but also provides no benefit to the static analysis here anyway. (None of the functions written here *assume* the shape of the structure, so there are no functions that benefit from having a more laboriously specified type.) If the comment needs more work, suggest away -- I tried to follow our last discussion here as best as I was able. >> +# here, because the purpose of this module is to interrogate that type. >> +_JSONObject = Dict[str, object] >> >> >> # Names consist of letters, digits, -, and _, starting with a letter. >> @@ -315,9 +324,20 @@ def check_event(expr, info): >> >> def check_exprs(exprs): >> for expr_elem in exprs: >> - expr = expr_elem['expr'] >> - info = expr_elem['info'] >> - doc = expr_elem.get('doc') >> + # Expression >> + assert isinstance(expr_elem['expr'], dict) > > I dislike relaxing OrderedDict to dict here. I'm going to accept it > anyway, because the difference between the two is going away in 3.7, and > because so far order actually matters only in certain sub-expressions, > not top-level expressions. > Right, there is a semantic piece of information missing from this type. You've asked if we care if we ditch OrderedDict and claim that we only support CPython -- I don't know. I assume nobody uses anything else, but I sincerely don't know. My hunch is that I really doubt it, but I don't see a reason to complicate ./configure et al and don't think it needs to be messed with. I am content with leaving OrderedDict in parser.py, but it does make it easier for me to play with the pieces to not impose a constraint on a specific *type name* and there is no way (that I am presently aware of) to write a type constraint on just the semantic information that the keys are ordered. The largest loss I am aware of here is that a newcomer to this file *may not know* that these keys are ordered, however, the order of the keys in and of itself has no impact on the operation of expr.py itself, so I am not sure if it is necessary to repeat that fact for a theoretical visitor here. parser.py of course still uses OrderedDict and will continue to do so for the forseeable future. "Why bother relaxing the type at all, then?" Strictly it makes life easier for me, because I am experimenting with different validation backends, different parsers, and so on. Can I just patch it out in every branch I want to play with these changes? I could, yes. I am asking for a favor in exchange for my continued diligence in adding documentation and static time type analysis to a critical component used for generating the API interface for QEMU. >> + for key in expr_elem['expr'].keys(): >> + assert isinstance(key, str) >> + expr: _JSONObject = expr_elem['expr'] >> + >> + # QAPISourceInfo >> + assert isinstance(expr_elem['info'], QAPISourceInfo) >> + info: QAPISourceInfo = expr_elem['info'] >> + >> + # Optional[QAPIDoc] >> + tmp = expr_elem.get('doc') >> + assert tmp is None or isinstance(tmp, QAPIDoc) >> + doc: Optional[QAPIDoc] = tmp >> >> if 'include' in expr: >> continue > > I see you didn't bite on the idea to do less checking here. Okay. > Almost all of this goes away in part 5, because I add a typed structure that parser returns and we no longer have to do the runtime type narrowing here as a result. I started to shift things around a bit here, but it'll just cause more work to rebase it again anyway, so I left it alone. I did reorder one of the other checks here, but wound up leaving this one alone. (I will admit to liking the assertion in the interim because it convinced me I was on terra-firma. Through all of the rebase churn, some more brain-dead looking bits help keep my expectations tethered to the current reality.)
On 3/25/21 10:26 AM, Markus Armbruster wrote:
> Suggest
>
> qapi/expr.py: Check type of union and alternate 'data' member
>
> John Snow <jsnow@redhat.com> writes:
>
>> We don't actually check, so the user can get some unpleasant stacktraces.
>
> Let's point to the new tests here.
>
Well, it'll get merged with the last one to keep make check working, so
I have to update the commit message anyway.
On 3/25/21 10:33 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Casts are instructions to the type checker only, they aren't "safe" and
>> should probably be avoided in general. In this case, when we perform
>> type checking on a nested structure, the type of each field does not
>> "stick".
>>
>> (See PEP 647 for an example of "type narrowing" that does "stick".
>> It is available in Python 3.10, so we can't use it yet.)
>>
>> We don't need to assert that something is a str if we've already checked
>> or asserted that it is -- use a cast instead for these cases.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/expr.py | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index ca5ab7bfda..505e67bd21 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -15,7 +15,7 @@
>> # See the COPYING file in the top-level directory.
>>
>> import re
>> -from typing import Dict, Optional
>> +from typing import Dict, Optional, cast
>>
>> from .common import c_name
>> from .error import QAPISemError
>> @@ -259,7 +259,7 @@ def check_enum(expr, info):
>>
>>
>> def check_struct(expr, info):
>> - name = expr['struct']
>> + name = cast(str, expr['struct']) # Asserted in check_exprs
>
> "Asserted" suggests an an assert statement. It's actually the
> check_name_is_str() visible in the last hunk. What about "Checked in
> check_exprs()" or "Ensured by check_exprs()"?
>
I missed these. "Checked" is fine.
On 3/25/21 10:45 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> This is a minor adjustment that allows the 'required' and 'optional'
>> keys fields to take a default value of an empty, immutable sequence (the
>> empty tuple).
>>
>> This reveals a quirk of this function, which is that "a + b" is
>> list-specific behavior. We can accept a wider variety of types if we
>> avoid that behavior. Using Collection allows us to accept things like
>> lists, tuples, sets, and so on.
>>
>> (Iterable would also have worked, but Iterable also includes things like
>> generator expressions which are consumed upon iteration, which would
>> require a rewrite to make sure that each input was only traversed once.)
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> The commit message confused me briefly, until I realized v3 of this
> patch came later in the series, where it modified check_keys() type
> hints and added default values.
>
> What about this:
>
> This is a minor adjustment that lets parameters @required and
> @optional take tuple arguments, in particular (). Later patches will
> make use of that.
>
OK
On 3/25/21 11:15 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> This is a small rewrite to address some minor style nits.
>>
>> Don't compare against the empty list to check for the empty condition, and
>> move the normalization forward to unify the check on the now-normalized
>> structure.
>>
>> With the check unified, the local nested function isn't needed anymore
>> and can be brought down into the normal flow of the function. With the
>> nesting level changed, shuffle the error strings around a bit to get
>> them to fit in 79 columns.
>>
>> Note: although ifcond is typed as Sequence[str] elsewhere, we *know* that
>> the parser will produce real, bona-fide lists. It's okay to check
>> isinstance(ifcond, list) here.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> scripts/qapi/expr.py | 32 ++++++++++++++------------------
>> 1 file changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index ea9d39fcf2..5921fa34ab 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -144,30 +144,26 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
>>
>> def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
>>
>> - def check_if_str(ifcond: object) -> None:
>> - if not isinstance(ifcond, str):
>> - raise QAPISemError(
>> - info,
>> - "'if' condition of %s must be a string or a list of strings"
>> - % source)
>> - if ifcond.strip() == '':
>> - raise QAPISemError(
>> - info,
>> - "'if' condition '%s' of %s makes no sense"
>> - % (ifcond, source))
>> -
>> ifcond = expr.get('if')
>> if ifcond is None:
>> return
>> +
>> if isinstance(ifcond, list):
>> - if ifcond == []:
>> + if not ifcond:
>> raise QAPISemError(
>> - info, "'if' condition [] of %s is useless" % source)
>> - for elt in ifcond:
>> - check_if_str(elt)
>> + info, f"'if' condition [] of {source} is useless")
>
> Unrelated change from interpolation to formatted string literal.
>
>> else:
>> - check_if_str(ifcond)
>> - expr['if'] = [ifcond]
>> + # Normalize to a list
>> + ifcond = expr['if'] = [ifcond]
>> +
>> + for elt in ifcond:
>> + if not isinstance(elt, str):
>> + raise QAPISemError(info, (
>> + f"'if' condition of {source}"
>> + " must be a string or a list of strings"))
>> + if not elt.strip():
>> + raise QAPISemError(
>> + info, f"'if' condition '{elt}' of {source} makes no sense")
>
> Likewise.
>
> I like formatted string literals, they're often easier to read than
> interpolation. But let's try to keep patches focused on their stated
> purpose.
>
> I'd gladly consider a series that convers to formatted strings
> wholesale. But I guess we better finish the typing job, first.
>
I am dreaming of a lush meadow.
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.
John Snow <jsnow@redhat.com> writes: > On 3/25/21 10:04 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> mypy does not know the types of values stored in Dicts that masquerade >>> as objects. Help the type checker out by constraining the type. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> scripts/qapi/expr.py | 26 +++++++++++++++++++++++--- >>> 1 file changed, 23 insertions(+), 3 deletions(-) >>> >>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >>> index b4bbcd54c0..b75c85c160 100644 >>> --- a/scripts/qapi/expr.py >>> +++ b/scripts/qapi/expr.py >>> @@ -15,9 +15,18 @@ >>> # See the COPYING file in the top-level directory. >>> >>> import re >>> +from typing import Dict, Optional >>> >>> from .common import c_name >>> from .error import QAPISemError >>> +from .parser import QAPIDoc >>> +from .source import QAPISourceInfo >>> + >>> + >>> +# Deserialized JSON objects as returned by the parser; >>> +# The values of this mapping are not necessary to exhaustively type >> >> Not necessary and also not practical with current mypy. Correct? > > Neither necessary nor practical. Typing as 'object' guarantees that > these values will never be used in a manner not supported by all python > objects. Mypy does not complain, so we know that we don't misuse the type. > > This is helpful for proving the validity of the expr.py validator > itself: we know that we are not forgetting to perform type narrowing and > using the value contained therein inappropriately. > > Adding a more exhaustive typing here is impractical (for reasons we > learned during introspect.py), but also provides no benefit to the > static analysis here anyway. > > (None of the functions written here *assume* the shape of the structure, > so there are no functions that benefit from having a more laboriously > specified type.) > > If the comment needs more work, suggest away -- I tried to follow our > last discussion here as best as I was able. "Needs more work" sounds like "inadequate", which isn't the case. The comment focuses on what we need from mypy here. We may or may not want to hint at the other aspect: what mypy can provide. >>> +# here, because the purpose of this module is to interrogate that type. >>> +_JSONObject = Dict[str, object] [...] If we want to, maybe: # Deserialized JSON objects as returned by the parser. # The values of this mapping are not necessary to exhaustively type # here (and also not practical as long as mypy lacks recursive # types), because the purpose of this module is to interrogate that # type. Thoughts?
John Snow <jsnow@redhat.com> writes:
> On 3/25/21 11:21 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> PEP8's BDFL writes: "For flowing long blocks of text with fewer
>>> structural restrictions (docstrings or comments), the line length should
>>> be limited to 72 characters."
>>>
>>> I do not like this patch. I have included it explicitly to recommend we
>>> do not pay any further heed to the 72 column limit.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> I'd like to get the remainder of this series moving again before digging
>> into this patch.
>
> I am dropping it, then -- I have no interest in bringing a patch I
> dislike along for another respin.
Despite your dislike, there might be good parts, and if there are, I'd
like to mine them. I don't need you to track the patch for that,
though. Feel free to drop it.
Thank you for exploring the max-doc-length option.
On 3/26/21 2:26 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 3/25/21 11:21 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> PEP8's BDFL writes: "For flowing long blocks of text with fewer
>>>> structural restrictions (docstrings or comments), the line length should
>>>> be limited to 72 characters."
>>>>
>>>> I do not like this patch. I have included it explicitly to recommend we
>>>> do not pay any further heed to the 72 column limit.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> I'd like to get the remainder of this series moving again before digging
>>> into this patch.
>>
>> I am dropping it, then -- I have no interest in bringing a patch I
>> dislike along for another respin.
>
> Despite your dislike, there might be good parts, and if there are, I'd
> like to mine them. I don't need you to track the patch for that,
> though. Feel free to drop it.
>
> Thank you for exploring the max-doc-length option.
>
Being less terse about it: Mostly, I don't like how it enforces this
column width even for indented structures. Generally, we claim that 72
columns is "comfortable to read" and I agree.
However, when we start in a margin, I
am not convinced that this is
actually more readable than the
alternative. We aren't using our full
72 characters here.
For personal projects I tend to relax the column limit to about 100
chars, which gives nice breathing room and generally reduces the edge
cases for error strings and so on. (Not suggesting we do that here so
long as we remain on a mailing-list based workflow.)
I can't say I am a fan of the limit; I don't think it's something I can
reasonably enforce for python/* so I have some concerns over
consistency, so I think it'd be easier to just not.
I *did* try, though; I just think it brought up too many judgment calls
for how to make single-line comments not look super awkward. I imagine
it'll cause similar delays for other authors, and exasperated sighs when
the CI fails due to a 73-column comment.
On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote:
> Being less terse about it: Mostly, I don't like how it enforces this
> column width even for indented structures. Generally, we claim that 72
> columns is "comfortable to read" and I agree.
>
> However, when we start in a margin, I
> am not convinced that this is
> actually more readable than the
> alternative. We aren't using our full
> 72 characters here.
I agree, and I don't see any strong reason to hold our Python
code to a different standard to the rest of our codebase as
regards line length and comment standards.
thanks
-- PMM
On 3/26/21 1:40 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 3/25/21 10:04 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> mypy does not know the types of values stored in Dicts that masquerade
>>>> as objects. Help the type checker out by constraining the type.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> scripts/qapi/expr.py | 26 +++++++++++++++++++++++---
>>>> 1 file changed, 23 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>> index b4bbcd54c0..b75c85c160 100644
>>>> --- a/scripts/qapi/expr.py
>>>> +++ b/scripts/qapi/expr.py
>>>> @@ -15,9 +15,18 @@
>>>> # See the COPYING file in the top-level directory.
>>>>
>>>> import re
>>>> +from typing import Dict, Optional
>>>>
>>>> from .common import c_name
>>>> from .error import QAPISemError
>>>> +from .parser import QAPIDoc
>>>> +from .source import QAPISourceInfo
>>>> +
>>>> +
>>>> +# Deserialized JSON objects as returned by the parser;
>>>> +# The values of this mapping are not necessary to exhaustively type
>>>
>>> Not necessary and also not practical with current mypy. Correct?
>>
>> Neither necessary nor practical. Typing as 'object' guarantees that
>> these values will never be used in a manner not supported by all python
>> objects. Mypy does not complain, so we know that we don't misuse the type.
>>
>> This is helpful for proving the validity of the expr.py validator
>> itself: we know that we are not forgetting to perform type narrowing and
>> using the value contained therein inappropriately.
>>
>> Adding a more exhaustive typing here is impractical (for reasons we
>> learned during introspect.py), but also provides no benefit to the
>> static analysis here anyway.
>>
>> (None of the functions written here *assume* the shape of the structure,
>> so there are no functions that benefit from having a more laboriously
>> specified type.)
>>
>> If the comment needs more work, suggest away -- I tried to follow our
>> last discussion here as best as I was able.
>
> "Needs more work" sounds like "inadequate", which isn't the case.
>
> The comment focuses on what we need from mypy here. We may or may not
> want to hint at the other aspect: what mypy can provide.
>
>>>> +# here, because the purpose of this module is to interrogate that type.
>>>> +_JSONObject = Dict[str, object]
> [...]
>
> If we want to, maybe:
>
> # Deserialized JSON objects as returned by the parser.
> # The values of this mapping are not necessary to exhaustively type
> # here (and also not practical as long as mypy lacks recursive
> # types), because the purpose of this module is to interrogate that
> # type.
>
> Thoughts?
>
If it occurs to you to want the extra explanation, I won't say no to it.
I will fold it in.
--js
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
Peter Maydell <peter.maydell@linaro.org> writes:
> On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote:
>> Being less terse about it: Mostly, I don't like how it enforces this
>> column width even for indented structures. Generally, we claim that 72
>> columns is "comfortable to read" and I agree.
>>
>> However, when we start in a margin, I
>> am not convinced that this is
>> actually more readable than the
>> alternative. We aren't using our full
>> 72 characters here.
>
> I agree, and I don't see any strong reason to hold our Python
> code to a different standard to the rest of our codebase as
> regards line length and comment standards.
I can't see much of a conflict between canonical Python style and the
rest of our code. (If there was a conflict, then I'd doubt we should
hold our Python code to a different standard than pretty much all the
other Python code out there.)
PEP 8 is expressly a *guide*. It doesn't want to be treated as law. It
tells you when to ignore its guidance even before it gives any, right in
the second section. Applicable part:
Some other good reasons to ignore a particular guideline:
1. When applying the guideline would make the code less readable,
even for someone who is used to reading code that follows this PEP.
Going beyond 72 colums to make the comment more readable is exactly what
PEP 8 wants you to do.
This is no excuse for going beyond when you could just as well break the
line earlier.
There's a reason pycodestyle distinguishes between errors and warnings,
and "line too long" is a warning. We've been conditioned to conflate
warnings with errors by C's "the standard permits it, but you really
shouldn't" warnings. However, treating style warnings as errors is
exactly what PEP 8 calls a folly of little minds.
John Snow <jsnow@redhat.com> writes: > On 3/26/21 2:26 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 3/25/21 11:21 AM, Markus Armbruster wrote: >>>> John Snow <jsnow@redhat.com> writes: >>>> >>>>> PEP8's BDFL writes: "For flowing long blocks of text with fewer >>>>> structural restrictions (docstrings or comments), the line length should >>>>> be limited to 72 characters." >>>>> >>>>> I do not like this patch. I have included it explicitly to recommend we >>>>> do not pay any further heed to the 72 column limit. >>>>> >>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> >>>> I'd like to get the remainder of this series moving again before digging >>>> into this patch. >>> >>> I am dropping it, then -- I have no interest in bringing a patch I >>> dislike along for another respin. >> Despite your dislike, there might be good parts, and if there are, >> I'd >> like to mine them. I don't need you to track the patch for that, >> though. Feel free to drop it. >> Thank you for exploring the max-doc-length option. >> > > Being less terse about it: Mostly, I don't like how it enforces this > column width even for indented structures. Generally, we claim that 72 > columns is "comfortable to read" and I agree. > > However, when we start in a margin, I > am not convinced that this is > actually more readable than the > alternative. We aren't using our full > 72 characters here. > > For personal projects I tend to relax the column limit to about 100 > chars, which gives nice breathing room and generally reduces the edge > cases for error strings and so on. (Not suggesting we do that here so > long as we remain on a mailing-list based workflow.) > > I can't say I am a fan of the limit; I don't think it's something I > can reasonably enforce for python/* so I have some concerns over > consistency, so I think it'd be easier to just not. I'm with PEP 8 here: go beyond the line length limits juidicously, not carelessly. This cannot be enforced automatically with the tools we have. > I *did* try, though; I just think it brought up too many judgment > calls for how to make single-line comments not look super awkward. I > imagine it'll cause similar delays for other authors, and exasperated > sighs when the CI fails due to a 73-column comment. Enforcing a hard 72 limit in CI would be precisely what PEP 8 does not want us to do.
On Fri, Mar 26, 2021 at 04:44:25PM +0000, Peter Maydell wrote: > On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote: > > Being less terse about it: Mostly, I don't like how it enforces this > > column width even for indented structures. Generally, we claim that 72 > > columns is "comfortable to read" and I agree. > > > > However, when we start in a margin, I > > am not convinced that this is > > actually more readable than the > > alternative. We aren't using our full > > 72 characters here. > > I agree, and I don't see any strong reason to hold our Python > code to a different standard to the rest of our codebase as > regards line length and comment standards. There's one small difference with python vs the rest of the codebase when it comes to API doc strings specifically. eg we have a docstring API comment in python/qemu/machine.py: class QEMUMachine: """ A QEMU VM. Use this object as a context manager to ensure the QEMU process terminates:: with VM(binary) as vm: ... # vm is guaranteed to be shut down here """ This formatting, including line breaks, is preserved as-is when a user requests viewing of the help: >>> print(help(qemu.machine.QEMUMachine)) Help on class QEMUMachine in module qemu.machine: class QEMUMachine(builtins.object) | QEMUMachine(binary: str, args: Sequence[str] = (), wrapper: Sequence[str] = (), name: Optional[str] = None, test_dir: str = '/var/tmp', monitor_address: Union[Tuple[str, str], str, NoneType] = None, socket_scm_helper: Optional[str] = None, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None) | | A QEMU VM. | | Use this object as a context manager to ensure | the QEMU process terminates:: | | with VM(binary) as vm: | ... | # vm is guaranteed to be shut down here | | Methods defined here: | IOW, while we as QEMU maintainers may not care about keeping to a narrow line width, with API docstrings, we're also declaring that none of the users of the python APIs can care either. These docstrings are never reflowed, so they can end up wrapping if the user's terminal is narrow which looks very ugly. So this python API docstring scenario is slightly different from our main codebase, where majority of comments are only ever going to be seen by QEMU maintainers, and where C API doc strings don't preserve formatting, because they're turned into HTML and re-flowed. Having said all that, I still don't think we need to restrict ourselves to 72 characters. This is not the 1980's with people using text terminals with physical size constraints. I think it is fine if we let python docstrings get larger - especially if the docstrings are already indented 4/8/12 spaces due to the code indent context, because the code indent is removed when comments are displayed. I think a 100 char line limit would be fine and still not cause wrapping when using python live help(). Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Mar 26, 2021 at 04:44:25PM +0000, Peter Maydell wrote: >> On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote: >> > Being less terse about it: Mostly, I don't like how it enforces this >> > column width even for indented structures. Generally, we claim that 72 >> > columns is "comfortable to read" and I agree. >> > >> > However, when we start in a margin, I >> > am not convinced that this is >> > actually more readable than the >> > alternative. We aren't using our full >> > 72 characters here. >> >> I agree, and I don't see any strong reason to hold our Python >> code to a different standard to the rest of our codebase as >> regards line length and comment standards. > > There's one small difference with python vs the rest of the codebase when > it comes to API doc strings specifically. eg we have a docstring API comment > in python/qemu/machine.py: > > class QEMUMachine: > """ > A QEMU VM. > > Use this object as a context manager to ensure > the QEMU process terminates:: > > with VM(binary) as vm: > ... > # vm is guaranteed to be shut down here > """ > > This formatting, including line breaks, is preserved as-is when a user > requests viewing of the help: > >>>> print(help(qemu.machine.QEMUMachine)) > > Help on class QEMUMachine in module qemu.machine: > > class QEMUMachine(builtins.object) > | QEMUMachine(binary: str, args: Sequence[str] = (), wrapper: Sequence[str] = (), name: Optional[str] = None, test_dir: str = '/var/tmp', monitor_address: Union[Tuple[str, str], str, NoneType] = None, socket_scm_helper: Optional[str] = None, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None) > | > | A QEMU VM. > | > | Use this object as a context manager to ensure > | the QEMU process terminates:: > | > | with VM(binary) as vm: > | ... > | # vm is guaranteed to be shut down here > | > | Methods defined here: > | > > > IOW, while we as QEMU maintainers may not care about keeping to a narrow > line width, with API docstrings, we're also declaring that none of the > users of the python APIs can care either. These docstrings are never > reflowed, so they can end up wrapping if the user's terminal is narrow > which looks very ugly. > > > So this python API docstring scenario is slightly different from our > main codebase, where majority of comments are only ever going to be seen > by QEMU maintainers, and where C API doc strings don't preserve formatting, > because they're turned into HTML and re-flowed. > > Having said all that, I still don't think we need to restrict ourselves > to 72 characters. This is not the 1980's with people using text terminals > with physical size constraints. I think it is fine if we let python > docstrings get larger - especially if the docstrings are already indented > 4/8/12 spaces due to the code indent context, because the code indent is > removed when comments are displayed. I think a 100 char line limit would > be fine and still not cause wrapping when using python live help(). The trouble with long lines is not text terminals, it's humans. Humans tend to have trouble following long lines with their eyes (I sure do). Typographic manuals suggest to limit columns to roughly 60 characters for exactly that reason[*]. Most doc strings are indented once (classes, functions) or twice (methods). 72 - 8 is roughly 60. With nesting, doc strings can become indented more. Nesting sufficient to squeeze the doc string width to column 72 under roughly 60 is pretty rare. Going beyond 72 colums to keep such doc strings readable is exactly what PEP 8 wants you to do. Again, I see no reason to deviate from PEP 8. [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style
On 4/9/21 5:33 AM, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Fri, Mar 26, 2021 at 04:44:25PM +0000, Peter Maydell wrote: >>> On Fri, 26 Mar 2021 at 16:33, John Snow <jsnow@redhat.com> wrote: >>>> Being less terse about it: Mostly, I don't like how it enforces this >>>> column width even for indented structures. Generally, we claim that 72 >>>> columns is "comfortable to read" and I agree. >>>> >>>> However, when we start in a margin, I >>>> am not convinced that this is >>>> actually more readable than the >>>> alternative. We aren't using our full >>>> 72 characters here. >>> >>> I agree, and I don't see any strong reason to hold our Python >>> code to a different standard to the rest of our codebase as >>> regards line length and comment standards. >> >> There's one small difference with python vs the rest of the codebase when >> it comes to API doc strings specifically. eg we have a docstring API comment >> in python/qemu/machine.py: >> >> class QEMUMachine: >> """ >> A QEMU VM. >> >> Use this object as a context manager to ensure >> the QEMU process terminates:: >> >> with VM(binary) as vm: >> ... >> # vm is guaranteed to be shut down here >> """ >> >> This formatting, including line breaks, is preserved as-is when a user >> requests viewing of the help: >> >>>>> print(help(qemu.machine.QEMUMachine)) >> >> Help on class QEMUMachine in module qemu.machine: >> >> class QEMUMachine(builtins.object) >> | QEMUMachine(binary: str, args: Sequence[str] = (), wrapper: Sequence[str] = (), name: Optional[str] = None, test_dir: str = '/var/tmp', monitor_address: Union[Tuple[str, str], str, NoneType] = None, socket_scm_helper: Optional[str] = None, sock_dir: Optional[str] = None, drain_console: bool = False, console_log: Optional[str] = None) >> | >> | A QEMU VM. >> | >> | Use this object as a context manager to ensure >> | the QEMU process terminates:: >> | >> | with VM(binary) as vm: >> | ... >> | # vm is guaranteed to be shut down here >> | >> | Methods defined here: >> | >> >> >> IOW, while we as QEMU maintainers may not care about keeping to a narrow >> line width, with API docstrings, we're also declaring that none of the >> users of the python APIs can care either. These docstrings are never >> reflowed, so they can end up wrapping if the user's terminal is narrow >> which looks very ugly. >> >> >> So this python API docstring scenario is slightly different from our >> main codebase, where majority of comments are only ever going to be seen >> by QEMU maintainers, and where C API doc strings don't preserve formatting, >> because they're turned into HTML and re-flowed. >> >> Having said all that, I still don't think we need to restrict ourselves >> to 72 characters. This is not the 1980's with people using text terminals >> with physical size constraints. I think it is fine if we let python >> docstrings get larger - especially if the docstrings are already indented >> 4/8/12 spaces due to the code indent context, because the code indent is >> removed when comments are displayed. I think a 100 char line limit would >> be fine and still not cause wrapping when using python live help(). > > The trouble with long lines is not text terminals, it's humans. Humans > tend to have trouble following long lines with their eyes (I sure do). > Typographic manuals suggest to limit columns to roughly 60 characters > for exactly that reason[*]. > > Most doc strings are indented once (classes, functions) or twice > (methods). 72 - 8 is roughly 60. > My problem with this patch isn't actually the docstrings -- it's one-line comments. If you can teach flake8 to allow this: # Pretend this is a single-line comment that's 73 chars but disallow this: # Pretend this is a two-line comment that's 73 chars, # and continues to a new line that's also pretty long, # and maybe keeps going, too. I will happily accept that patch. Without the ability to enforce the style though, I am reluctant to pretend that it's even a preference that we have. I think it's a waste to hunt down and re-flow single-line comments that just barely squeak over a limit. They look worse. We can discuss this more when we go to propose a style guide for the Python folder; I think it's maybe a misprioritization of our energies in the present context. (I still have the style guide on my TODO list, and even began writing a draft at one point, but I think we'd both like to press forward on the Typing bits first.) > With nesting, doc strings can become indented more. Nesting sufficient > to squeeze the doc string width to column 72 under roughly 60 is pretty > rare. Going beyond 72 colums to keep such doc strings readable is > exactly what PEP 8 wants you to do. > > Again, I see no reason to deviate from PEP 8. > > > [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style >
John Snow <jsnow@redhat.com> writes: > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 213 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 208 insertions(+), 5 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 1869ddf815..adc5b903bc 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -1,7 +1,5 @@ > # -*- coding: utf-8 -*- > # > -# Check (context-free) QAPI schema expression structure > -# > # Copyright IBM, Corp. 2011 > # Copyright (c) 2013-2019 Red Hat Inc. > # > @@ -14,6 +12,25 @@ > # This work is licensed under the terms of the GNU GPL, version 2. > # See the COPYING file in the top-level directory. > > +""" > +Normalize and validate (context-free) QAPI schema expression structures. > + > +After QAPI expressions are parsed from disk, they are stored in > +recursively nested Python data structures using Dict, List, str, bool, > +and int. This module ensures that those nested structures have the > +correct type(s) and key(s) where appropriate for the QAPI context-free > +grammar. "from disk"? Perhaps something like "QAPISchemaParser parses the QAPI schema into abstract syntax trees consisting of dict, list, str, bool and int nodes." This isn't quite accurate; it parses into a list of {'expr': AST, 'info': INFO}, but that's detail. PEP 8: You should use two spaces after a sentence-ending period in multi- sentence comments, except after the final sentence. > + > +The QAPI schema expression language allows for syntactic sugar; this suggest "certain syntactic sugar". > +module also handles the normalization process of these nested > +structures. > + > +See `check_exprs` for the main entry point. > + > +See `schema.QAPISchema` for processing into native Python data > +structures and contextual semantic validation. > +""" > + > import re > from typing import ( > Collection, > @@ -31,9 +48,10 @@ > from .source import QAPISourceInfo > > > -# Deserialized JSON objects as returned by the parser; > -# The values of this mapping are not necessary to exhaustively type > -# here, because the purpose of this module is to interrogate that type. > +#: Deserialized JSON objects as returned by the parser. > +#: > +#: The values of this mapping are not necessary to exhaustively type > +#: here, because the purpose of this module is to interrogate that type. First time I see #: comments; pardon my ignorance. What's the deal? > _JSONObject = Dict[str, object] > > > @@ -48,11 +66,29 @@ > def check_name_is_str(name: object, > info: QAPISourceInfo, > source: str) -> None: > + """Ensures that ``name`` is a string.""" PEP 257: The docstring [...] prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ...". More of the same below. > if not isinstance(name, str): > raise QAPISemError(info, "%s requires a string name" % source) > > > def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: > + """ > + Ensures a string is a legal name. I feel "legal" is best reserved to matters of law. Suggest "valid QAPI name". More of the same below. > + > + A legal name consists of ascii letters, digits, ``-``, and ``_``, ASCII > + starting with a letter. The names of downstream extensions are > + prefixed with an __com.example_ style prefix, allowing ``.`` and > + ``-``. An experimental name is prefixed with ``x-``, following the > + RFQDN if present. I get that "an _com.experimental style prefix" and "the RFQDN" mean the same thing, but can we make this clearer? Perhaps A valid name consists of ascii letters, digits, ``-``, and ``_``, starting with a letter. It may be prefixed by a downstream prefix of the form __RFQDN_, or the experimental prefix ``x-``. If both prefixes are present, the __RFQDN_ prefix goes first. I'm clueless on proper use of `` in doc strings. Can you educate me? > + > + A legal name cannot start with ``q_``, which is reserved. > + > + :param name: Name to check. > + :param info: QAPI source file information. Suggest to say "QAPI schema source information", or maybe "QAPI schema source file information". > + :param source: Human-readable str describing "what" this name is. Could mention it's for use in error messages, but we have many similar parameters all over the place, so this would perhaps be more tiresome than helpful. Fine as is. > + > + :return: The stem of the valid name, with no prefixes. > + """ > # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' > # and 'q_obj_*' implicit type names. > match = valid_name.match(name) > @@ -62,6 +98,12 @@ def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: > > > def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: > + """ > + Ensures a string is a legal event name. > + > + Checks the same criteria as `check_name_str`, but requires uppercase > + and prohibits ``-``. > + """ > stem = check_name_str(name, info, source) > if re.search(r'[a-z-]', stem): > raise QAPISemError( > @@ -71,6 +113,15 @@ def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: > def check_name_lower(name: str, info: QAPISourceInfo, source: str, > permit_upper: bool = False, > permit_underscore: bool = False) -> None: > + """ > + Ensures a string is a legal user defined type name. > + > + Checks the same criteria as `check_name_str`, but may impose > + additional constraints. Correct, but it leads to slightly awkward "permit_FOO: prohibit ... when false": > + > + :param permit_upper: Prohibits uppercase when false. > + :param permit_underscore: Prohibits underscores when false. > + """ Perhaps something like Ensure @str is a valid command or member name. This means it must be a valid QAPI name (as ensured by check_name_str()), where the stem consists of lowercase characters and '-'. :param permit_upper: Additionally permit uppercase. :param permit_underscore: Additionally permit '_'. We'd then want to update check_name_upper() and check_name_camel() for consistency. > stem = check_name_str(name, info, source) > if ((not permit_upper and re.search(r'[A-Z]', stem)) > or (not permit_underscore and '_' in stem)): > @@ -79,12 +130,31 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, > > > def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None: > + """ > + Ensures a string is a legal CamelCase name. > + > + Checks the same criteria as `check_name_str`, > + but additionally imposes a CamelCase constraint. > + """ > stem = check_name_str(name, info, source) > if not re.match(r'[A-Z][A-Za-z0-9]*[a-z][A-Za-z0-9]*$', stem): > raise QAPISemError(info, "name of %s must use CamelCase" % source) Related: we discussed renaming check_name_{upper,camel,lower} to check_name_{event,type,other} or check_name_{event,user_defined_type, command_or_member}. > > > def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None: > + """ > + Ensures a name is a legal definition name. > + > + - 'event' names adhere to `check_name_upper`. > + - 'command' names adhere to `check_name_lower`. > + - All other names adhere to `check_name_camel`. When is a name an 'event' name? We should spell out how this uses @meta. Perhaps something like: - If meta == 'event', ... - If meta == 'command, ... - Else, meta is a type, and ... > + > + All name types must not end with ``Kind`` nor ``List``. Do you mean "type names"? Outside this patch's scope: qapi-code-gen.txt reserves suffixes Kind and List only for type names, but the code rejects them for events and commands, too. One of them should be changed to match the other. > + > + :param name: Name to check. > + :param info: QAPI source file information. > + :param meta: Type name of the QAPI expression. > + """ Glosses over the use of pragma command-name-exceptions. Do we mind? > if meta == 'event': > check_name_upper(name, info, meta) > elif meta == 'command': > @@ -103,6 +173,15 @@ def check_keys(value: _JSONObject, > source: str, > required: Collection[str], > optional: Collection[str]) -> None: > + """ > + Ensures an object has a specific set of keys. > + > + :param value: The object to check. > + :param info: QAPI source file information. > + :param source: Human-readable str describing this value. > + :param required: Keys that *must* be present. > + :param optional: Keys that *may* be present. > + """ > > def pprint(elems: Iterable[str]) -> str: > return ', '.join("'" + e + "'" for e in sorted(elems)) > @@ -125,6 +204,12 @@ def pprint(elems: Iterable[str]) -> str: > > > def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: > + """ > + Ensures common fields in an expression are correct. Rather vague. The function is really about checking "flag" members, i.e. members that must have a boolean value. Perhaps Ensure flag members (if present) have valid values. > + > + :param expr: Expression to validate. > + :param info: QAPI source file information. > + """ > for key in ['gen', 'success-response']: > if key in expr and expr[key] is not False: > raise QAPISemError( > @@ -143,7 +228,22 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: > > > def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: > + """ > + Syntactically validate and normalize the ``if`` field of an object. qapi-code-gen.txt consistently uses "member", not "field". Let's stick to "member". > > + The ``if`` field may be either a ``str`` or a ``List[str]``. > + A ``str`` element will be normalized to ``List[str]``. element? I think you mean value. Doesn't spell out how str is normalized to List[str], but I guess that's obvious enough. > + > + :forms: > + :sugared: ``Union[str, List[str]]`` > + :canonical: ``List[str]`` Uh... :param FOO: and :return: are obvious enough, but this :forms: stuff feels a bit too fancy for me to rely on voodoo understanding. Can you point me to :documentation:? > + > + :param expr: A ``dict``. Elsewhere, you have "the object to check", which I like better. > + The ``if`` field, if present, will be validated. > + :param info: QAPI source file information. > + > + :return: None, ``expr`` is normalized in-place as needed. > + """ > ifcond = expr.get('if') > if ifcond is None: > return > @@ -167,6 +267,20 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: > > > def normalize_members(members: object) -> None: > + """ > + Normalize a "members" value. > + > + If ``members`` is an object, for every value in that object, if that If it's a dict, actually. > + value is not itself already an object, normalize it to > + ``{'type': value}``. > + > + :forms: > + :sugared: ``Dict[str, Union[str, TypeRef]]`` > + :canonical: ``Dict[str, TypeRef]`` > + > + :param members: The members object to normalize. > + :return: None, ``members`` is normalized in-place as needed. > + """ > if isinstance(members, dict): > for key, arg in members.items(): > if isinstance(arg, dict): > @@ -179,6 +293,23 @@ def check_type(value: Optional[object], > source: str, > allow_array: bool = False, > allow_dict: Union[bool, str] = False) -> None: > + """ > + Check the QAPI type of ``value``. > + > + Python types of ``str`` or ``None`` are always allowed. > + > + :param value: The value to check. > + :param info: QAPI Source file information. > + :param source: Human-readable str describing this value. > + :param allow_array: Allow a ``List[str]`` of length 1, > + which indicates an Array<T> type. Leaves open where T comes from. Perhaps "indicates an array of the type named by the list element". > + :param allow_dict: Allow a dict, treated as an anonymous type. "treated as anonymous type" isn't quite right. The dict is either MEMBERS or BRANCHES in qapi-code-gen.txt parlance. The former is like an anonymous struct type, the latter isn't. > + When given a string, check if that name is > + allowed to have keys that use uppercase letters, > + and modify validation accordingly. The second sentence feels both cryptic and vague. > + > + :return: None, ``value`` is normalized in-place as needed. First mention of normalization. I think we normalize only dicts. Perhaps: :param allow_dict: Allow a dict. Its members can be struct type members or union branches. When the value of @allow_dict is in pragma member-name-exceptions, the dict's keys may violate the member naming rules. The dict members are normalized in place. Still neglects to explain we normalize. Also note the indentation style: it produces reasonable text width regardless of parameter name length. Slightly different way to get that: :param allow_dict: Allow a dict. Its members can be struct type members or union branches. When the value of @allow_dict is in pragma member-name-exceptions, the dict's keys may violate the member naming rules. The dict members are normalized in place. > + """ > if value is None: > return > > @@ -227,6 +358,21 @@ def check_type(value: Optional[object], > > def check_features(features: Optional[object], > info: QAPISourceInfo) -> None: > + """ > + Syntactically validate and normalize the ``features`` field. > + > + ``features`` may be a ``list`` of either ``str`` or ``dict``. > + Any ``str`` element will be normalized to ``{'name': element}``. > + > + :forms: > + :sugared: ``List[Union[str, Feature]]`` > + :canonical: ``List[Feature]`` > + > + :param features: an optional list of either str or dict. > + :param info: QAPI Source file information. > + > + :return: None, ``features`` is normalized in-place as needed. > + """ > if features is None: > return > if not isinstance(features, list): > @@ -244,6 +390,14 @@ def check_features(features: Optional[object], > > > def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: > + """ > + Validate this expression as an ``enum`` definition. > + > + :param expr: The expression to validate. > + :param info: QAPI source file information. > + > + :return: None, ``expr`` is normalized in-place as needed. > + """ Unlike the one for check_features(), this one doesn't describe how we normalize. More of the same below. Observation, not demand. > name = expr['enum'] > members = expr['data'] > prefix = expr.get('prefix') > @@ -273,6 +427,14 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: > > > def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: > + """ > + Validate this expression as a ``struct`` definition. > + > + :param expr: The expression to validate. > + :param info: QAPI source file information. > + > + :return: None, ``expr`` is normalized in-place as needed. > + """ > name = cast(str, expr['struct']) # Asserted in check_exprs > members = expr['data'] > > @@ -281,6 +443,14 @@ def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: > > > def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: > + """ > + Validate this expression as a ``union`` definition. > + > + :param expr: The expression to validate. > + :param info: QAPI source file information. > + > + :return: None, ``expr`` is normalized in-place as needed. > + """ > name = cast(str, expr['union']) # Asserted in check_exprs > base = expr.get('base') > discriminator = expr.get('discriminator') > @@ -309,6 +479,14 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: > > > def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: > + """ > + Validate this expression as an ``alternate`` definition. > + > + :param expr: The expression to validate. > + :param info: QAPI source file information. > + > + :return: None, ``expr`` is normalized in-place as needed. > + """ > members = expr['data'] > > if not members: > @@ -326,6 +504,14 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: > > > def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: > + """ > + Validate this expression as a ``command`` definition. > + > + :param expr: The expression to validate. > + :param info: QAPI source file information. > + > + :return: None, ``expr`` is normalized in-place as needed. > + """ > args = expr.get('data') > rets = expr.get('returns') > boxed = expr.get('boxed', False) > @@ -337,6 +523,14 @@ def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: > > > def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: > + """ > + Normalize and validate this expression as an ``event`` definition. > + > + :param expr: The expression to validate. > + :param info: QAPI source file information. > + > + :return: None, ``expr`` is normalized in-place as needed. > + """ > args = expr.get('data') > boxed = expr.get('boxed', False) > > @@ -346,6 +540,15 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: > > > def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: > + """ > + Validate and normalize a list of parsed QAPI schema expressions. > + > + This function accepts a list of expressions + metadta as returned by Typo: metadata. > + the parser. It destructively normalizes the expressions in-place. > + > + :param exprs: The list of expressions to normalize/validate. > + :return: The same list of expressions (now modified). > + """ > for expr_elem in exprs: > # Expression > assert isinstance(expr_elem['expr'], dict)
John Snow <jsnow@redhat.com> writes: > PEP8's BDFL writes: "For flowing long blocks of text with fewer > structural restrictions (docstrings or comments), the line length should > be limited to 72 characters." > > I do not like this patch. I have included it explicitly to recommend we > do not pay any further heed to the 72 column limit. Let me go through the patch hunk by hunk to see what I like and what I don't like. In case you'd prefer not to pay any further heed to line length: please check out my comment on c_name() anyway. It's about doc string style, and relevant regardless of line length limits. > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/.flake8 | 1 + > scripts/qapi/common.py | 8 +++++--- > scripts/qapi/events.py | 9 +++++---- > scripts/qapi/gen.py | 8 ++++---- > scripts/qapi/introspect.py | 8 +++++--- > scripts/qapi/main.py | 4 ++-- > scripts/qapi/parser.py | 15 ++++++++------- > scripts/qapi/schema.py | 23 +++++++++++++---------- > scripts/qapi/types.py | 7 ++++--- > 9 files changed, 47 insertions(+), 36 deletions(-) > > diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8 > index 6b158c68b8..4f00455290 100644 > --- a/scripts/qapi/.flake8 > +++ b/scripts/qapi/.flake8 > @@ -1,2 +1,3 @@ > [flake8] > extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's > +max-doc-length = 72 > \ No newline at end of file Since we intend to make use of PEP 8's license to go over the line length limit, having the build gripe about it is not useful. Drop. > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index cbd3fd81d3..6e3d9b8ecd 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -41,7 +41,8 @@ def camel_to_upper(value: str) -> str: > length = len(c_fun_str) > for i in range(length): > char = c_fun_str[i] > - # When char is upper case and no '_' appears before, do more checks > + # When char is upper case and no '_' appears before, > + # do more checks > if char.isupper() and (i > 0) and c_fun_str[i - 1] != '_': > if i < length - 1 and c_fun_str[i + 1].islower(): > new_name += '_' The comment paraphrases the if condition. Feels useless. Let's drop it. > @@ -78,8 +79,9 @@ def c_name(name: str, protect: bool = True) -> str: > protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' > > :param name: The name to map. > - :param protect: If true, avoid returning certain ticklish identifiers > - (like C keywords) by prepending ``q_``. > + :param protect: If true, avoid returning certain ticklish > + identifiers (like C keywords) by prepending > + ``q_``. Better: :param protect: If true, avoid returning certain ticklish identifiers (like C keywords) by prepending ``q_``. For what it's worth, this indentation style is also used in the Sphinx-RTD-Tutorial[*]. I like it much better than aligning the text like you did, because that wastes screen real estate when the parameter names are long, and tempts people to aligning all the parameters, like :param name: The name to map. :param protect: If true, avoid returning certain ticklish identifiers (like C keywords) by prepending ``q_``. which leads to either churn or inconsistency when parameters with longer names get added. > """ > # ANSI X3J11/88-090, 3.1.1 > c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py > index fee8c671e7..210b56974f 100644 > --- a/scripts/qapi/events.py > +++ b/scripts/qapi/events.py > @@ -48,7 +48,8 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str: > """ > Generate a struct variable holding the event parameters. > > - Initialize it with the function arguments defined in `gen_event_send`. > + Initialize it with the function arguments defined in > + `gen_event_send`. > """ > assert not typ.variants > ret = mcgen(''' Looks like a wash. I figure the doc string will be rewritten to Sphinx format (or whatever other format we adopt for our Python code) anyway, so let's not mess with it now. > @@ -86,9 +87,9 @@ def gen_event_send(name: str, > # FIXME: Our declaration of local variables (and of 'errp' in the > # parameter list) can collide with exploded members of the event's > # data type passed in as parameters. If this collision ever hits in > - # practice, we can rename our local variables with a leading _ prefix, > - # or split the code into a wrapper function that creates a boxed > - # 'param' object then calls another to do the real work. > + # practice, we can rename our local variables with a leading _ > + # prefix, or split the code into a wrapper function that creates a > + # boxed 'param' object then calls another to do the real work. > have_args = boxed or (arg_type and not arg_type.is_empty()) > > ret = mcgen(''' Improvement. > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py > index 1fa503bdbd..c54980074e 100644 > --- a/scripts/qapi/gen.py > +++ b/scripts/qapi/gen.py > @@ -63,9 +63,9 @@ def _bottom(self) -> str: > return '' > > def write(self, output_dir: str) -> None: > - # Include paths starting with ../ are used to reuse modules of the main > - # schema in specialised schemas. Don't overwrite the files that are > - # already generated for the main schema. > + # Include paths starting with ../ are used to reuse modules > + # of the main schema in specialised schemas. Don't overwrite > + # the files that are already generated for the main schema. > if self.fname.startswith('../'): > return > pathname = os.path.join(output_dir, self.fname) Improvement, but mind PEP 8's "You should use two spaces after a sentence-ending period in multi-sentence comments". > @@ -189,7 +189,7 @@ def _bottom(self) -> str: > @contextmanager > def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]: > """ > - A with-statement context manager that wraps with `start_if()` / `end_if()`. > + A context manager that wraps output with `start_if()` / `end_if()`. > > :param ifcond: A sequence of conditionals, passed to `start_if()`. > :param args: any number of `QAPIGenCCode`. Improvement. > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 9a348ca2e5..faf00013ad 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -61,8 +61,9 @@ > # With optional annotations, the type of all values is: > # JSONValue = Union[_Value, Annotated[_Value]] > # > -# Sadly, mypy does not support recursive types; so the _Stub alias is used to > -# mark the imprecision in the type model where we'd otherwise use JSONValue. > +# Sadly, mypy does not support recursive types; so the _Stub alias is > +# used to mark the imprecision in the type model where we'd otherwise > +# use JSONValue. > _Stub = Any > _Scalar = Union[str, bool, None] > _NonScalar = Union[Dict[str, _Stub], List[_Stub]] Improvement. > @@ -217,7 +218,8 @@ def visit_end(self) -> None: > self._name_map = {} > > def visit_needed(self, entity: QAPISchemaEntity) -> bool: > - # Ignore types on first pass; visit_end() will pick up used types > + # Ignore types on first pass; > + # visit_end() will pick up used types Looks a bit odd. Since the original is only slightly over the limit, we can keep it. Alternatively. # Ignore types on first pass; visit_end() will pick up the # types that are actually used > return not isinstance(entity, QAPISchemaType) > > def _name(self, name: str) -> str: > diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py > index 703e7ed1ed..5bcac83985 100644 > --- a/scripts/qapi/main.py > +++ b/scripts/qapi/main.py > @@ -1,5 +1,5 @@ > -# This work is licensed under the terms of the GNU GPL, version 2 or later. > -# See the COPYING file in the top-level directory. > +# This work is licensed under the terms of the GNU GPL, version 2 or > +# later. See the COPYING file in the top-level directory. Let's drop this one. The line is only slightly too long, and consistency with the copright notices elsewhere is more important. > > """ > QAPI Generator > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 58267c3db9..d5bf91f2b0 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -331,8 +331,8 @@ def __init__(self, parser, name=None, indent=0): > self._indent = indent > > def append(self, line): > - # Strip leading spaces corresponding to the expected indent level > - # Blank lines are always OK. > + # Strip leading spaces corresponding to the expected indent > + # level. Blank lines are always OK. > if line: > indent = re.match(r'\s*', line).end() > if indent < self._indent: Improvement, but mind PEP 8's "You should use two spaces after a sentence-ending period". > @@ -353,10 +353,10 @@ def connect(self, member): > self.member = member > > def __init__(self, parser, info): > - # self._parser is used to report errors with QAPIParseError. The > - # resulting error position depends on the state of the parser. > - # It happens to be the beginning of the comment. More or less > - # servicable, but action at a distance. > + # self._parser is used to report errors with QAPIParseError. > + # The resulting error position depends on the state of the > + # parser. It happens to be the beginning of the comment. More > + # or less servicable, but action at a distance. > self._parser = parser > self.info = info > self.symbol = None Why not. Two spaces again. > @@ -430,7 +430,8 @@ def _append_body_line(self, line): > if not line.endswith(':'): > raise QAPIParseError(self._parser, "line should end with ':'") > self.symbol = line[1:-1] > - # FIXME invalid names other than the empty string aren't flagged > + # FIXME invalid names other than the empty string aren't > + # flagged > if not self.symbol: > raise QAPIParseError(self._parser, "invalid name") > elif self.symbol: Not an improvement, drop the hunk. > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 703b446fd2..01cdd753cd 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -166,9 +166,10 @@ def is_user_module(cls, name: str) -> bool: > @classmethod > def is_builtin_module(cls, name: str) -> bool: > """ > - The built-in module is a single System module for the built-in types. > + Return true when given the built-in module name. > > - It is always "./builtin". > + The built-in module is a specific System module for the built-in > + types. It is always "./builtin". > """ > return name == cls.BUILTIN_MODULE_NAME > I figure the doc string will be rewritten to Sphinx format anyway, so let's not mess with it now. > @@ -294,7 +295,8 @@ def connect_doc(self, doc=None): > m.connect_doc(doc) > > def is_implicit(self): > - # See QAPISchema._make_implicit_enum_type() and ._def_predefineds() > + # See QAPISchema._make_implicit_enum_type() and > + # ._def_predefineds() > return self.name.endswith('Kind') or self.name == 'QType' > > def c_type(self): Not an improvement, drop the hunk. > @@ -421,9 +423,9 @@ def check(self, schema): > > self.members = members # mark completed > > - # Check that the members of this type do not cause duplicate JSON members, > - # and update seen to track the members seen so far. Report any errors > - # on behalf of info, which is not necessarily self.info > + # Check that the members of this type do not cause duplicate JSON > + # members, and update seen to track the members seen so far. Report > + # any errors on behalf of info, which is not necessarily self.info > def check_clash(self, info, seen): > assert self._checked > assert not self.variants # not implemented Improvement. Two spaces again. > @@ -494,11 +496,12 @@ def __init__(self, name, info, doc, ifcond, features, variants): > def check(self, schema): > super().check(schema) > self.variants.tag_member.check(schema) > - # Not calling self.variants.check_clash(), because there's nothing > - # to clash with > + # Not calling self.variants.check_clash(), because there's > + # nothing to clash with > self.variants.check(schema, {}) > - # Alternate branch names have no relation to the tag enum values; > - # so we have to check for potential name collisions ourselves. > + # Alternate branch names have no relation to the tag enum > + # values; so we have to check for potential name collisions > + # ourselves. > seen = {} > types_seen = {} > for v in self.variants.variants: Why not. > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index 20d572a23a..2e67ab1752 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -35,8 +35,8 @@ > from .source import QAPISourceInfo > > > -# variants must be emitted before their container; track what has already > -# been output > +# variants must be emitted before their container; track what has > +# already been output > objects_seen = set() > > Why not. > @@ -297,7 +297,8 @@ def _begin_user_module(self, name: str) -> None: > ''')) > > def visit_begin(self, schema: QAPISchema) -> None: > - # gen_object() is recursive, ensure it doesn't visit the empty type > + # gen_object() is recursive, ensure > + # it doesn't visit the empty type Looks a bit odd. Since the original is only slightly over the limit, we can keep it. Pattern: turning single line comments into multi-line comments to avoid small length overruns is usually not an improvement. > objects_seen.add(schema.the_empty_object_type.name) > > def _gen_type_cleanup(self, name: str) -> None: Bottom line: I find some hunks likable enough. Ways forward: 1. If you need to respin: 1.1. you may keep this patch, and work in my feedback. 1.2. you may drop it. I can pick it up and take care of it. 2. If we decide to go without a respin: 2.1. I can work in my feedback in my tree. 2.2. I can extract the patch and take care of it separately. I'd prefer to avoid 2.1, because I feel it's too much change for comfort. 1.1. vs. 1.2 would be up to you. [*] https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html#an-example-class-with-docstrings
On 4/16/21 8:44 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> PEP8's BDFL writes: "For flowing long blocks of text with fewer >> structural restrictions (docstrings or comments), the line length should >> be limited to 72 characters." >> >> I do not like this patch. I have included it explicitly to recommend we >> do not pay any further heed to the 72 column limit. > > Let me go through the patch hunk by hunk to see what I like and what I > don't like. > > In case you'd prefer not to pay any further heed to line length: please > check out my comment on c_name() anyway. It's about doc string style, > and relevant regardless of line length limits. > Right, yeah. I just don't think this is productive right now. I'll read it, though! >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/.flake8 | 1 + >> scripts/qapi/common.py | 8 +++++--- >> scripts/qapi/events.py | 9 +++++---- >> scripts/qapi/gen.py | 8 ++++---- >> scripts/qapi/introspect.py | 8 +++++--- >> scripts/qapi/main.py | 4 ++-- >> scripts/qapi/parser.py | 15 ++++++++------- >> scripts/qapi/schema.py | 23 +++++++++++++---------- >> scripts/qapi/types.py | 7 ++++--- >> 9 files changed, 47 insertions(+), 36 deletions(-) >> >> diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8 >> index 6b158c68b8..4f00455290 100644 >> --- a/scripts/qapi/.flake8 >> +++ b/scripts/qapi/.flake8 >> @@ -1,2 +1,3 @@ >> [flake8] >> extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's >> +max-doc-length = 72 >> \ No newline at end of file > > Since we intend to make use of PEP 8's license to go over the line > length limit, having the build gripe about it is not useful. Drop. > >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index cbd3fd81d3..6e3d9b8ecd 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -41,7 +41,8 @@ def camel_to_upper(value: str) -> str: >> length = len(c_fun_str) >> for i in range(length): >> char = c_fun_str[i] >> - # When char is upper case and no '_' appears before, do more checks >> + # When char is upper case and no '_' appears before, >> + # do more checks >> if char.isupper() and (i > 0) and c_fun_str[i - 1] != '_': >> if i < length - 1 and c_fun_str[i + 1].islower(): >> new_name += '_' > > The comment paraphrases the if condition. Feels useless. Let's drop > it. > >> @@ -78,8 +79,9 @@ def c_name(name: str, protect: bool = True) -> str: >> protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' >> >> :param name: The name to map. >> - :param protect: If true, avoid returning certain ticklish identifiers >> - (like C keywords) by prepending ``q_``. >> + :param protect: If true, avoid returning certain ticklish >> + identifiers (like C keywords) by prepending >> + ``q_``. > > Better: > > :param protect: If true, avoid returning certain ticklish > identifiers (like C keywords) by prepending ``q_``. > > For what it's worth, this indentation style is also used in the > Sphinx-RTD-Tutorial[*]. I like it much better than aligning the text > like you did, because that wastes screen real estate when the parameter > names are long, and tempts people to aligning all the parameters, like > > :param name: The name to map. > :param protect: If true, avoid returning certain ticklish identifiers > (like C keywords) by prepending ``q_``. > > which leads to either churn or inconsistency when parameters with longer > names get added. > Yeah, that should be fine. I don't like the wasted margin space either, but I suppose I like the "two column" layout for ease of visual distinction of the parameter names. I suppose it isn't really worth the kind of column-reformatting churn and the wasted space. ...And if we do print a sphinx manual for this, I'll get my visual distinction there in the rendered output. I'm fine with adopting this style to cover the entire Python codebase. It will be an eventual thing, though: I think we need to agree on a style guide document and in that same series, fix up the instances of defying that guide. I think it's important to pair that work, because the ease of finding and fixing those style deviations will help inform how pragmatic the style guide is. I feel like it's something I want to do very soon, but not right now. Maybe during the next freeze we can tackle it? >> """ >> # ANSI X3J11/88-090, 3.1.1 >> c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', >> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py >> index fee8c671e7..210b56974f 100644 >> --- a/scripts/qapi/events.py >> +++ b/scripts/qapi/events.py >> @@ -48,7 +48,8 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str: >> """ >> Generate a struct variable holding the event parameters. >> >> - Initialize it with the function arguments defined in `gen_event_send`. >> + Initialize it with the function arguments defined in >> + `gen_event_send`. >> """ >> assert not typ.variants >> ret = mcgen(''' > > Looks like a wash. I figure the doc string will be rewritten to Sphinx > format (or whatever other format we adopt for our Python code) anyway, > so let's not mess with it now. > >> @@ -86,9 +87,9 @@ def gen_event_send(name: str, >> # FIXME: Our declaration of local variables (and of 'errp' in the >> # parameter list) can collide with exploded members of the event's >> # data type passed in as parameters. If this collision ever hits in >> - # practice, we can rename our local variables with a leading _ prefix, >> - # or split the code into a wrapper function that creates a boxed >> - # 'param' object then calls another to do the real work. >> + # practice, we can rename our local variables with a leading _ >> + # prefix, or split the code into a wrapper function that creates a >> + # boxed 'param' object then calls another to do the real work. >> have_args = boxed or (arg_type and not arg_type.is_empty()) >> >> ret = mcgen(''' > > Improvement. > >> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >> index 1fa503bdbd..c54980074e 100644 >> --- a/scripts/qapi/gen.py >> +++ b/scripts/qapi/gen.py >> @@ -63,9 +63,9 @@ def _bottom(self) -> str: >> return '' >> >> def write(self, output_dir: str) -> None: >> - # Include paths starting with ../ are used to reuse modules of the main >> - # schema in specialised schemas. Don't overwrite the files that are >> - # already generated for the main schema. >> + # Include paths starting with ../ are used to reuse modules >> + # of the main schema in specialised schemas. Don't overwrite >> + # the files that are already generated for the main schema. >> if self.fname.startswith('../'): >> return >> pathname = os.path.join(output_dir, self.fname) > > Improvement, but mind PEP 8's "You should use two spaces after a > sentence-ending period in multi-sentence comments". > How important is this, and why? My existing prejudice is that it's only a superficial detail of writing with no real impact. (Of course, a single space typist WOULD believe that, wouldn't they? Those single-space typists are all the same!) >> @@ -189,7 +189,7 @@ def _bottom(self) -> str: >> @contextmanager >> def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]: >> """ >> - A with-statement context manager that wraps with `start_if()` / `end_if()`. >> + A context manager that wraps output with `start_if()` / `end_if()`. >> >> :param ifcond: A sequence of conditionals, passed to `start_if()`. >> :param args: any number of `QAPIGenCCode`. > > Improvement. > >> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >> index 9a348ca2e5..faf00013ad 100644 >> --- a/scripts/qapi/introspect.py >> +++ b/scripts/qapi/introspect.py >> @@ -61,8 +61,9 @@ >> # With optional annotations, the type of all values is: >> # JSONValue = Union[_Value, Annotated[_Value]] >> # >> -# Sadly, mypy does not support recursive types; so the _Stub alias is used to >> -# mark the imprecision in the type model where we'd otherwise use JSONValue. >> +# Sadly, mypy does not support recursive types; so the _Stub alias is >> +# used to mark the imprecision in the type model where we'd otherwise >> +# use JSONValue. >> _Stub = Any >> _Scalar = Union[str, bool, None] >> _NonScalar = Union[Dict[str, _Stub], List[_Stub]] > > Improvement. > >> @@ -217,7 +218,8 @@ def visit_end(self) -> None: >> self._name_map = {} >> >> def visit_needed(self, entity: QAPISchemaEntity) -> bool: >> - # Ignore types on first pass; visit_end() will pick up used types >> + # Ignore types on first pass; >> + # visit_end() will pick up used types > > Looks a bit odd. Since the original is only slightly over the limit, we > can keep it. Alternatively. > > # Ignore types on first pass; visit_end() will pick up the > # types that are actually used > >> return not isinstance(entity, QAPISchemaType) >> >> def _name(self, name: str) -> str: >> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py >> index 703e7ed1ed..5bcac83985 100644 >> --- a/scripts/qapi/main.py >> +++ b/scripts/qapi/main.py >> @@ -1,5 +1,5 @@ >> -# This work is licensed under the terms of the GNU GPL, version 2 or later. >> -# See the COPYING file in the top-level directory. >> +# This work is licensed under the terms of the GNU GPL, version 2 or >> +# later. See the COPYING file in the top-level directory. > > Let's drop this one. The line is only slightly too long, and > consistency with the copright notices elsewhere is more important. > >> >> """ >> QAPI Generator >> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >> index 58267c3db9..d5bf91f2b0 100644 >> --- a/scripts/qapi/parser.py >> +++ b/scripts/qapi/parser.py >> @@ -331,8 +331,8 @@ def __init__(self, parser, name=None, indent=0): >> self._indent = indent >> >> def append(self, line): >> - # Strip leading spaces corresponding to the expected indent level >> - # Blank lines are always OK. >> + # Strip leading spaces corresponding to the expected indent >> + # level. Blank lines are always OK. >> if line: >> indent = re.match(r'\s*', line).end() >> if indent < self._indent: > > Improvement, but mind PEP 8's "You should use two spaces after a > sentence-ending period". > >> @@ -353,10 +353,10 @@ def connect(self, member): >> self.member = member >> >> def __init__(self, parser, info): >> - # self._parser is used to report errors with QAPIParseError. The >> - # resulting error position depends on the state of the parser. >> - # It happens to be the beginning of the comment. More or less >> - # servicable, but action at a distance. >> + # self._parser is used to report errors with QAPIParseError. >> + # The resulting error position depends on the state of the >> + # parser. It happens to be the beginning of the comment. More >> + # or less servicable, but action at a distance. >> self._parser = parser >> self.info = info >> self.symbol = None > > Why not. Two spaces again. > >> @@ -430,7 +430,8 @@ def _append_body_line(self, line): >> if not line.endswith(':'): >> raise QAPIParseError(self._parser, "line should end with ':'") >> self.symbol = line[1:-1] >> - # FIXME invalid names other than the empty string aren't flagged >> + # FIXME invalid names other than the empty string aren't >> + # flagged >> if not self.symbol: >> raise QAPIParseError(self._parser, "invalid name") >> elif self.symbol: > > Not an improvement, drop the hunk. > >> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >> index 703b446fd2..01cdd753cd 100644 >> --- a/scripts/qapi/schema.py >> +++ b/scripts/qapi/schema.py >> @@ -166,9 +166,10 @@ def is_user_module(cls, name: str) -> bool: >> @classmethod >> def is_builtin_module(cls, name: str) -> bool: >> """ >> - The built-in module is a single System module for the built-in types. >> + Return true when given the built-in module name. >> >> - It is always "./builtin". >> + The built-in module is a specific System module for the built-in >> + types. It is always "./builtin". >> """ >> return name == cls.BUILTIN_MODULE_NAME >> > > I figure the doc string will be rewritten to Sphinx format anyway, so > let's not mess with it now. > >> @@ -294,7 +295,8 @@ def connect_doc(self, doc=None): >> m.connect_doc(doc) >> >> def is_implicit(self): >> - # See QAPISchema._make_implicit_enum_type() and ._def_predefineds() >> + # See QAPISchema._make_implicit_enum_type() and >> + # ._def_predefineds() >> return self.name.endswith('Kind') or self.name == 'QType' >> >> def c_type(self): > > Not an improvement, drop the hunk. > >> @@ -421,9 +423,9 @@ def check(self, schema): >> >> self.members = members # mark completed >> >> - # Check that the members of this type do not cause duplicate JSON members, >> - # and update seen to track the members seen so far. Report any errors >> - # on behalf of info, which is not necessarily self.info >> + # Check that the members of this type do not cause duplicate JSON >> + # members, and update seen to track the members seen so far. Report >> + # any errors on behalf of info, which is not necessarily self.info >> def check_clash(self, info, seen): >> assert self._checked >> assert not self.variants # not implemented > > Improvement. Two spaces again. > >> @@ -494,11 +496,12 @@ def __init__(self, name, info, doc, ifcond, features, variants): >> def check(self, schema): >> super().check(schema) >> self.variants.tag_member.check(schema) >> - # Not calling self.variants.check_clash(), because there's nothing >> - # to clash with >> + # Not calling self.variants.check_clash(), because there's >> + # nothing to clash with >> self.variants.check(schema, {}) >> - # Alternate branch names have no relation to the tag enum values; >> - # so we have to check for potential name collisions ourselves. >> + # Alternate branch names have no relation to the tag enum >> + # values; so we have to check for potential name collisions >> + # ourselves. >> seen = {} >> types_seen = {} >> for v in self.variants.variants: > > Why not. > >> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py >> index 20d572a23a..2e67ab1752 100644 >> --- a/scripts/qapi/types.py >> +++ b/scripts/qapi/types.py >> @@ -35,8 +35,8 @@ >> from .source import QAPISourceInfo >> >> >> -# variants must be emitted before their container; track what has already >> -# been output >> +# variants must be emitted before their container; track what has >> +# already been output >> objects_seen = set() >> >> > > Why not. > >> @@ -297,7 +297,8 @@ def _begin_user_module(self, name: str) -> None: >> ''')) >> >> def visit_begin(self, schema: QAPISchema) -> None: >> - # gen_object() is recursive, ensure it doesn't visit the empty type >> + # gen_object() is recursive, ensure >> + # it doesn't visit the empty type > > Looks a bit odd. Since the original is only slightly over the limit, we > can keep it. > > Pattern: turning single line comments into multi-line comments to avoid > small length overruns is usually not an improvement. > Yep, that's my core argument against turning on the option for flake8. Usually rephrasing is better than re-flowing, but that wasn't always easy either. (I don't like rewriting things to be less terse, I find it unpleasant, sorry!) Unfortunately, omitting it from flake8 means I'll probably also miss cases where I or someone else have gone slightly over the limit for docstrings, and doubt it will be enforced consistently. "Patches welcome" as the old curse goes. >> objects_seen.add(schema.the_empty_object_type.name) >> >> def _gen_type_cleanup(self, name: str) -> None: > > Bottom line: I find some hunks likable enough. > > Ways forward: > > 1. If you need to respin: > > 1.1. you may keep this patch, and work in my feedback. > > 1.2. you may drop it. I can pick it up and take care of it. > This one, please! I have to admit that my appetite for consistency runs out right around here, but I'll never reject someone else doing this kind of work if they find it important. You may also wish to look into the Python packaging series at some point, as you may be able to augment the tests to provide a "manual" run that produces some extra warnings from time to time that you may want to address, which you might find helpful for pursuing these kinds of cleanups in the future where I suspect they will inevitably regress. > 2. If we decide to go without a respin: > > 2.1. I can work in my feedback in my tree. > > 2.2. I can extract the patch and take care of it separately. > > I'd prefer to avoid 2.1, because I feel it's too much change for > comfort. 1.1. vs. 1.2 would be up to you. > > > > [*] https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html#an-example-class-with-docstrings >
On 4/14/21 11:04 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > Thanks for taking this on. I realize it's a slog. (Update: much later: AUUUGHHHHH WHY DID I DECIDE TO WRITE DOCS. MY HUBRIS) >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/expr.py | 213 ++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 208 insertions(+), 5 deletions(-) >> >> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >> index 1869ddf815..adc5b903bc 100644 >> --- a/scripts/qapi/expr.py >> +++ b/scripts/qapi/expr.py >> @@ -1,7 +1,5 @@ >> # -*- coding: utf-8 -*- >> # >> -# Check (context-free) QAPI schema expression structure >> -# >> # Copyright IBM, Corp. 2011 >> # Copyright (c) 2013-2019 Red Hat Inc. >> # >> @@ -14,6 +12,25 @@ >> # This work is licensed under the terms of the GNU GPL, version 2. >> # See the COPYING file in the top-level directory. >> >> +""" >> +Normalize and validate (context-free) QAPI schema expression structures. >> + >> +After QAPI expressions are parsed from disk, they are stored in >> +recursively nested Python data structures using Dict, List, str, bool, >> +and int. This module ensures that those nested structures have the >> +correct type(s) and key(s) where appropriate for the QAPI context-free >> +grammar. > > "from disk"? Perhaps something like "QAPISchemaParser parses the QAPI > schema into abstract syntax trees consisting of dict, list, str, bool > and int nodes." This isn't quite accurate; it parses into a list of > {'expr': AST, 'info': INFO}, but that's detail. > Let's skip the detail; it doesn't help communicate purpose in the first paragraph here at the high level. The bulk of this module IS primarily concerned with the structures named. Edited to: `QAPISchemaParser` parses a QAPI schema into abstract syntax trees consisting of dict, list, str, bool, and int nodes. This module ensures that these nested structures have the correct type(s) and key(s) where appropriate for the QAPI context-free grammar. (I replaced "the QAPI schema" with "a QAPI schema" as we have several; and I wanted to hint at (somehow) that this processes configurable input (i.e. "from disk") and not something indelibly linked.) ((What's wrong with "from disk?")) > PEP 8: You should use two spaces after a sentence-ending period in > multi- sentence comments, except after the final sentence. > Is this a demand? >> + >> +The QAPI schema expression language allows for syntactic sugar; this > > suggest "certain syntactic sugar". > OK >> +module also handles the normalization process of these nested >> +structures. >> + >> +See `check_exprs` for the main entry point. >> + >> +See `schema.QAPISchema` for processing into native Python data >> +structures and contextual semantic validation. >> +""" >> + >> import re >> from typing import ( >> Collection, >> @@ -31,9 +48,10 @@ >> from .source import QAPISourceInfo >> >> >> -# Deserialized JSON objects as returned by the parser; >> -# The values of this mapping are not necessary to exhaustively type >> -# here, because the purpose of this module is to interrogate that type. >> +#: Deserialized JSON objects as returned by the parser. >> +#: >> +#: The values of this mapping are not necessary to exhaustively type >> +#: here, because the purpose of this module is to interrogate that type. > > First time I see #: comments; pardon my ignorance. What's the deal? > Sphinx-ese: It indicates that this comment is actually a block relating to the entity below. It also means that I can cross-reference `_JSONObject` in docstrings. ... which, because of the rewrite where I stopped calling this object an Expression to avoid overloading a term, is something I actually don't try to cross-reference anymore. So this block can be dropped now, actually. (Also: It came up in part one, actually: I snuck one in for EATSPACE, and reference it in the comment for cgen. We cannot cross-reference constants unless they are documented, and this is how we accomplish that.) >> _JSONObject = Dict[str, object] >> >> >> @@ -48,11 +66,29 @@ >> def check_name_is_str(name: object, >> info: QAPISourceInfo, >> source: str) -> None: >> + """Ensures that ``name`` is a string.""" > > PEP 257: The docstring [...] prescribes the function or method's effect > as a command ("Do this", "Return that"), not as a description; > e.g. don't write "Returns the pathname ...". > > More of the same below. > Alright. While we're here, then ... I take this to mean that you prefer: :raise: to :raises:, and :return: to :returns: ? And since I need to adjust the verb anyway, I might as well use "Check" instead of "Ensure". """ Check that ``name`` is a string. :raise: QAPISemError when ``name`` is an incorrect type. """ which means our preferred spellings should be: :param: (and not parameter, arg, argument, key, keyword) :raise: (and not raises, except, exception) :var/ivar/cvar: (variable, instance variable, class variable) :return: (and not returns) Disallow these, as covered by the mypy signature: :type: :vartype: :rtype: >> if not isinstance(name, str): >> raise QAPISemError(info, "%s requires a string name" % source) >> >> >> def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: >> + """ >> + Ensures a string is a legal name. > > I feel "legal" is best reserved to matters of law. Suggest "valid QAPI > name". > > More of the same below. > Yep, that's the type of review I like here. Getting the terms exactly correct. You've usually gone through some length to be consistent in your own writing, but it doesn't always stick with me. (I want a jargon file...) >> + >> + A legal name consists of ascii letters, digits, ``-``, and ``_``, > > ASCII > >> + starting with a letter. The names of downstream extensions are >> + prefixed with an __com.example_ style prefix, allowing ``.`` and >> + ``-``. An experimental name is prefixed with ``x-``, following the >> + RFQDN if present. > > I get that "an _com.experimental style prefix" and "the RFQDN" mean the > same thing, but can we make this clearer? Perhaps > > A valid name consists of ascii letters, digits, ``-``, and ``_``, > starting with a letter. It may be prefixed by a downstream > prefix of the form __RFQDN_, or the experimental prefix ``x-``. > If both prefixes are present, the __RFQDN_ prefix goes first. > "It may be prefixed by a downstream prefix" seems redundant, but no better ideas. Adopted your phrasing wholesale. > I'm clueless on proper use of `` in doc strings. Can you educate me? > It's just markup to get "literal" text. In practice, it renders in monospace. I may not have a great internal barometer for when it should be used or not. Some tendencies: 1. When referring to literal symbols without wanting to invoke any specific string literal syntax between languages, e.g. 'A' vs "A" might work well as ``A``. 2. When referring to parameter names, to intuit that this is a proper noun in the code. (The @foo syntax you use in qapi-doc is resolved to the equivalent of ``foo``when we generate those docs.) 3. Generally whenever I need to highlight symbols like ' " ` . _ - that might get confused for other punctuation, might accidentally render as markup, or otherwise need some kind of dressing. Whenever the noun I want to address is something cross-referencable, I will instead use `thing` and Sphinx will decorate that reference as it deems appropriate for the type of symbol that it is. >> + >> + A legal name cannot start with ``q_``, which is reserved. >> + >> + :param name: Name to check. >> + :param info: QAPI source file information. > > Suggest to say "QAPI schema source information", or maybe "QAPI schema > source file information". > OK >> + :param source: Human-readable str describing "what" this name is. > > Could mention it's for use in error messages, but we have many similar > parameters all over the place, so this would perhaps be more tiresome > than helpful. Fine as is. > Yes, I struggled because I think it doesn't have a consistent "type" of string that it is -- sometimes it's just the name of the definition type, sometimes it's a small phrase. Grammatically, I guess it is the subject of the error sentence. If you have a concrete suggestion, I'll take it. Otherwise, I'm just gonna make it worse. >> + >> + :return: The stem of the valid name, with no prefixes. >> + """ >> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' >> # and 'q_obj_*' implicit type names. >> match = valid_name.match(name) >> @@ -62,6 +98,12 @@ def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: >> >> >> def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: >> + """ >> + Ensures a string is a legal event name. >> + >> + Checks the same criteria as `check_name_str`, but requires uppercase >> + and prohibits ``-``. >> + """ >> stem = check_name_str(name, info, source) >> if re.search(r'[a-z-]', stem): >> raise QAPISemError( >> @@ -71,6 +113,15 @@ def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: >> def check_name_lower(name: str, info: QAPISourceInfo, source: str, >> permit_upper: bool = False, >> permit_underscore: bool = False) -> None: >> + """ >> + Ensures a string is a legal user defined type name. >> + >> + Checks the same criteria as `check_name_str`, but may impose >> + additional constraints. > > Correct, but it leads to slightly awkward "permit_FOO: prohibit ... when > false": > >> + >> + :param permit_upper: Prohibits uppercase when false. >> + :param permit_underscore: Prohibits underscores when false. >> + """ > > Perhaps something like > > Ensure @str is a valid command or member name. > > This means it must be a valid QAPI name (as ensured by > check_name_str()), where the stem consists of lowercase > characters and '-'. > > :param permit_upper: Additionally permit uppercase. > :param permit_underscore: Additionally permit '_'. > > We'd then want to update check_name_upper() and check_name_camel() for > consistency. > """ Check that ``name`` is a valid user defined type name. This means it must be a valid QAPI name as checked by `check_name_str()`, but where the stem prohibits uppercase characters and ``_``. :param permit_upper: Additionally permit uppercase. :param permit_underscore: Additionally permit ``_``. """ Using "but where" to highlight the differences, and removing the parenthetical to make the "see also" more clear to avoid repeating a paraphrase of what a valid QAPI name is. Using literals to highlight @name, because otherwise there is no processing here that will do the same for us. It may be possible to extend Sphinx so that it will do it for us, if you are attached to that visual signifier in the code itself. >> stem = check_name_str(name, info, source) >> if ((not permit_upper and re.search(r'[A-Z]', stem)) >> or (not permit_underscore and '_' in stem)): >> @@ -79,12 +130,31 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, >> >> >> def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None: >> + """ >> + Ensures a string is a legal CamelCase name. >> + >> + Checks the same criteria as `check_name_str`, >> + but additionally imposes a CamelCase constraint. >> + """ >> stem = check_name_str(name, info, source) >> if not re.match(r'[A-Z][A-Za-z0-9]*[a-z][A-Za-z0-9]*$', stem): >> raise QAPISemError(info, "name of %s must use CamelCase" % source) > > Related: we discussed renaming check_name_{upper,camel,lower} to > check_name_{event,type,other} or check_name_{event,user_defined_type, > command_or_member}. > Yep, it's a *little* funny to have check_lower(allow_upper=True) ... but I am happy with a doc for now. """ Check that ``name`` is a valid command or member name. This means it must be a valid QAPI name as checked by `check_name_str()`, but where the stem must be in CamelCase. """ >> >> >> def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None: >> + """ >> + Ensures a name is a legal definition name. >> + >> + - 'event' names adhere to `check_name_upper`. >> + - 'command' names adhere to `check_name_lower`. >> + - All other names adhere to `check_name_camel`. > > When is a name an 'event' name? We should spell out how this uses > @meta. Perhaps something like: > > - If meta == 'event', ... > - If meta == 'command, ... > - Else, meta is a type, and ... > >> + >> + All name types must not end with ``Kind`` nor ``List``. > > Do you mean "type names"? > I meant "all categories of names". "All names must not end with ``Kind`` nor ``List``." > Outside this patch's scope: qapi-code-gen.txt reserves suffixes Kind and > List only for type names, but the code rejects them for events and > commands, too. One of them should be changed to match the other. > Instinct is that code should change to match the "spec", as it probably best represents your intent. Laziness suggests that updating the "spec" means I don't have to write new tests. >> + >> + :param name: Name to check. >> + :param info: QAPI source file information. >> + :param meta: Type name of the QAPI expression. >> + """ > > Glosses over the use of pragma command-name-exceptions. Do we mind? > Mmmm. Nah? I think if you're digging that deep by now you'll have noticed that check_name_lower() takes some parameters, so the shorter statement isn't lying. It just isn't telling you exactly how the parameters are decided. >> if meta == 'event': >> check_name_upper(name, info, meta) >> elif meta == 'command': >> @@ -103,6 +173,15 @@ def check_keys(value: _JSONObject, >> source: str, >> required: Collection[str], >> optional: Collection[str]) -> None: >> + """ >> + Ensures an object has a specific set of keys. >> + >> + :param value: The object to check. >> + :param info: QAPI source file information. >> + :param source: Human-readable str describing this value. >> + :param required: Keys that *must* be present. >> + :param optional: Keys that *may* be present. >> + """ >> Style check: Avoid the two-column approach for stuff like this, too? Also, reminder to self, update the phrasing on ALL :param info: statements. >> def pprint(elems: Iterable[str]) -> str: >> return ', '.join("'" + e + "'" for e in sorted(elems)) >> @@ -125,6 +204,12 @@ def pprint(elems: Iterable[str]) -> str: >> >> >> def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: >> + """ >> + Ensures common fields in an expression are correct. > > Rather vague. The function is really about checking "flag" members, > i.e. members that must have a boolean value. Perhaps > > Ensure flag members (if present) have valid values. > Done! >> + >> + :param expr: Expression to validate. >> + :param info: QAPI source file information. >> + """ >> for key in ['gen', 'success-response']: >> if key in expr and expr[key] is not False: >> raise QAPISemError( >> @@ -143,7 +228,22 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: >> >> >> def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: >> + """ >> + Syntactically validate and normalize the ``if`` field of an object. > > qapi-code-gen.txt consistently uses "member", not "field". Let's stick > to "member". > Good, thanks. >> >> + The ``if`` field may be either a ``str`` or a ``List[str]``. >> + A ``str`` element will be normalized to ``List[str]``. > > element? I think you mean value. > Err, yeah. because... it's a single element ... of the list we're gonna make. Get it? (: (Fixed.) > Doesn't spell out how str is normalized to List[str], but I guess that's > obvious enough. > >> + >> + :forms: >> + :sugared: ``Union[str, List[str]]`` >> + :canonical: ``List[str]`` > > Uh... :param FOO: and :return: are obvious enough, but this :forms: > stuff feels a bit too fancy for me to rely on voodoo understanding. Can > you point me to :documentation:? > Haha. https://docutils.sourceforge.io/docs/user/rst/quickref.html#field-lists The "canonical" field lists we use are just special ones that have been bookmarked by Sphinx as having special significance. You can use others arbitrarily, if you want. This nests them to achieve a multi-column thing. Forms: Sugared: Foo Canonical: Bar >> + >> + :param expr: A ``dict``. > > Elsewhere, you have "the object to check", which I like better. > I agree. I was not careful about not accidentally repeating type information where it wasn't necessary. Semantic descriptions and not mechanical descriptions are certainly preferred. Fixed! >> + The ``if`` field, if present, will be validated. >> + :param info: QAPI source file information. >> + >> + :return: None, ``expr`` is normalized in-place as needed. >> + """ >> ifcond = expr.get('if') >> if ifcond is None: >> return >> @@ -167,6 +267,20 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: >> >> >> def normalize_members(members: object) -> None: >> + """ >> + Normalize a "members" value. >> + >> + If ``members`` is an object, for every value in that object, if that > > If it's a dict, actually. > Sigh, yeah. Working at the boundary between two languages is going to murder what's left of my sanity, I can feel it. >> + value is not itself already an object, normalize it to >> + ``{'type': value}``. >> + >> + :forms: >> + :sugared: ``Dict[str, Union[str, TypeRef]]`` >> + :canonical: ``Dict[str, TypeRef]`` >> + >> + :param members: The members object to normalize. >> + :return: None, ``members`` is normalized in-place as needed. >> + """ >> if isinstance(members, dict): >> for key, arg in members.items(): >> if isinstance(arg, dict): >> @@ -179,6 +293,23 @@ def check_type(value: Optional[object], >> source: str, >> allow_array: bool = False, >> allow_dict: Union[bool, str] = False) -> None: >> + """ >> + Check the QAPI type of ``value``. >> + >> + Python types of ``str`` or ``None`` are always allowed. >> + >> + :param value: The value to check. >> + :param info: QAPI Source file information. >> + :param source: Human-readable str describing this value. >> + :param allow_array: Allow a ``List[str]`` of length 1, >> + which indicates an Array<T> type. > > Leaves open where T comes from. Perhaps "indicates an array of the type > named by the list element". > Fair. >> + :param allow_dict: Allow a dict, treated as an anonymous type. > > "treated as anonymous type" isn't quite right. The dict is either > MEMBERS or BRANCHES in qapi-code-gen.txt parlance. The former is like > an anonymous struct type, the latter isn't. > Oh, yes. Ehm. >> + When given a string, check if that name is >> + allowed to have keys that use uppercase letters, >> + and modify validation accordingly. > > The second sentence feels both cryptic and vague. > This weird ol' function signature is not done torturing me. Maybe: :param allow_dict: Allow a dict, which represents a union branch or an anonymous struct type. This parameter may be given the struct or union name ``value`` under consideration. In this case, the name is used to conditionally allow less strict name validation, based on `QAPISchemaPragma`. (Oh, you suggested a fix below. Oops.) Argh. Maybe I'll just 'fix' this to have a slightly more laborious signature. def check_type(value: Optional[object], info: QAPISourceInfo, source: str, allow_array: bool = False, allow_dict: bool = False, defn_name: str = '') and then - permissive = False - if isinstance(allow_dict, str): - permissive = allow_dict in info.pragma.member_name_exceptions + permissive = defn_name and defn_name in info.pragma.member_name_exceptions and callers just have to specify both: check_type(..., allow_dict=True, defn_name=name) and then say: :param allow_dict: Allow ``value`` to be a dict, representing a union branch or an anonymous struct type. :param defn_name: The struct or union name under consideration. Used to conditionally allow more permissive member name validation based on `QAPISchemaPragma`. Stuff for later? >> + >> + :return: None, ``value`` is normalized in-place as needed. > > First mention of normalization. I think we normalize only dicts. > No, I also use that term in the docstrings for this module, check_if, and normalize_members above. (Maybe your review is non-linear. No problem.) I consider desugaring a form of input normalization. I have mentioned it for :return: to suggest that although there is no return value on the stack, the value passed (or a descendant thereof) *may* be modified. (That is, this isn't "just" a check function, it CAN modify your input!) It may occur here because of our call to check_if(). > Perhaps: > > :param allow_dict: Allow a dict. Its members can be struct type > members or union branches. When the value of @allow_dict is > in pragma member-name-exceptions, the dict's keys may violate > the member naming rules. The dict members are normalized in > place. > > Still neglects to explain we normalize. > > Also note the indentation style: it produces reasonable text width > regardless of parameter name length. Slightly different way to get > that: > > :param allow_dict: > Allow a dict. Its members can be struct type members or > union branches. When the value of @allow_dict is in pragma > member-name-exceptions, the dict's keys may violate the > member naming rules. The dict members are normalized in > place. > Oh, I like that style a lot -- it helps preserve the "term / definition" visual distinction. I may adopt that for any definition longer than a single-line. I'll probably adopt it for this patch and beyond. >> + """ >> if value is None: >> return >> >> @@ -227,6 +358,21 @@ def check_type(value: Optional[object], >> >> def check_features(features: Optional[object], >> info: QAPISourceInfo) -> None: >> + """ >> + Syntactically validate and normalize the ``features`` field. >> + >> + ``features`` may be a ``list`` of either ``str`` or ``dict``. >> + Any ``str`` element will be normalized to ``{'name': element}``. >> + >> + :forms: >> + :sugared: ``List[Union[str, Feature]]`` >> + :canonical: ``List[Feature]`` >> + >> + :param features: an optional list of either str or dict. >> + :param info: QAPI Source file information. >> + >> + :return: None, ``features`` is normalized in-place as needed. >> + """ >> if features is None: >> return >> if not isinstance(features, list): >> @@ -244,6 +390,14 @@ def check_features(features: Optional[object], >> >> >> def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: >> + """ >> + Validate this expression as an ``enum`` definition. >> + >> + :param expr: The expression to validate. >> + :param info: QAPI source file information. >> + >> + :return: None, ``expr`` is normalized in-place as needed. >> + """ > > Unlike the one for check_features(), this one doesn't describe how we > normalize. More of the same below. Observation, not demand. > I didn't mention specifics because another helper actually does the work; it's done through descendant calls on fields that may only be optionally present. I tried to keep a consistent phrasing for it. >> name = expr['enum'] >> members = expr['data'] >> prefix = expr.get('prefix') >> @@ -273,6 +427,14 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: >> >> >> def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: >> + """ >> + Validate this expression as a ``struct`` definition. >> + >> + :param expr: The expression to validate. >> + :param info: QAPI source file information. >> + >> + :return: None, ``expr`` is normalized in-place as needed. >> + """ >> name = cast(str, expr['struct']) # Asserted in check_exprs >> members = expr['data'] >> >> @@ -281,6 +443,14 @@ def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: >> >> >> def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: >> + """ >> + Validate this expression as a ``union`` definition. >> + >> + :param expr: The expression to validate. >> + :param info: QAPI source file information. >> + >> + :return: None, ``expr`` is normalized in-place as needed. >> + """ >> name = cast(str, expr['union']) # Asserted in check_exprs >> base = expr.get('base') >> discriminator = expr.get('discriminator') >> @@ -309,6 +479,14 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: >> >> >> def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: >> + """ >> + Validate this expression as an ``alternate`` definition. >> + >> + :param expr: The expression to validate. >> + :param info: QAPI source file information. >> + >> + :return: None, ``expr`` is normalized in-place as needed. >> + """ >> members = expr['data'] >> >> if not members: >> @@ -326,6 +504,14 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: >> >> >> def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: >> + """ >> + Validate this expression as a ``command`` definition. >> + >> + :param expr: The expression to validate. >> + :param info: QAPI source file information. >> + >> + :return: None, ``expr`` is normalized in-place as needed. >> + """ >> args = expr.get('data') >> rets = expr.get('returns') >> boxed = expr.get('boxed', False) >> @@ -337,6 +523,14 @@ def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: >> >> >> def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: >> + """ >> + Normalize and validate this expression as an ``event`` definition. >> + >> + :param expr: The expression to validate. >> + :param info: QAPI source file information. >> + >> + :return: None, ``expr`` is normalized in-place as needed. >> + """ >> args = expr.get('data') >> boxed = expr.get('boxed', False) >> >> @@ -346,6 +540,15 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: >> >> >> def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: >> + """ >> + Validate and normalize a list of parsed QAPI schema expressions. >> + >> + This function accepts a list of expressions + metadta as returned by > > Typo: metadata. > I've invented a new kind of data, actually. (Fixed.) >> + the parser. It destructively normalizes the expressions in-place. >> + >> + :param exprs: The list of expressions to normalize/validate. >> + :return: The same list of expressions (now modified). >> + """ >> for expr_elem in exprs: >> # Expression >> assert isinstance(expr_elem['expr'], dict) Made a bunch of changes, but held off on trying to "finish" it; I want to make a checklist for myself to review with your counter-feedback and methodically revise it all in one shot. Thanks! --js
John Snow <jsnow@redhat.com> writes: > On 4/16/21 8:44 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> PEP8's BDFL writes: "For flowing long blocks of text with fewer >>> structural restrictions (docstrings or comments), the line length should >>> be limited to 72 characters." >>> >>> I do not like this patch. I have included it explicitly to recommend we >>> do not pay any further heed to the 72 column limit. >> >> Let me go through the patch hunk by hunk to see what I like and what I >> don't like. >> >> In case you'd prefer not to pay any further heed to line length: please >> check out my comment on c_name() anyway. It's about doc string style, >> and relevant regardless of line length limits. >> > > Right, yeah. I just don't think this is productive right now. I'll read > it, though! Thanks! >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> scripts/qapi/.flake8 | 1 + >>> scripts/qapi/common.py | 8 +++++--- >>> scripts/qapi/events.py | 9 +++++---- >>> scripts/qapi/gen.py | 8 ++++---- >>> scripts/qapi/introspect.py | 8 +++++--- >>> scripts/qapi/main.py | 4 ++-- >>> scripts/qapi/parser.py | 15 ++++++++------- >>> scripts/qapi/schema.py | 23 +++++++++++++---------- >>> scripts/qapi/types.py | 7 ++++--- >>> 9 files changed, 47 insertions(+), 36 deletions(-) >>> >>> diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8 >>> index 6b158c68b8..4f00455290 100644 >>> --- a/scripts/qapi/.flake8 >>> +++ b/scripts/qapi/.flake8 >>> @@ -1,2 +1,3 @@ >>> [flake8] >>> extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's >>> +max-doc-length = 72 >>> \ No newline at end of file >> >> Since we intend to make use of PEP 8's license to go over the line >> length limit, having the build gripe about it is not useful. Drop. >> >>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>> index cbd3fd81d3..6e3d9b8ecd 100644 >>> --- a/scripts/qapi/common.py >>> +++ b/scripts/qapi/common.py >>> @@ -41,7 +41,8 @@ def camel_to_upper(value: str) -> str: >>> length = len(c_fun_str) >>> for i in range(length): >>> char = c_fun_str[i] >>> - # When char is upper case and no '_' appears before, do more checks >>> + # When char is upper case and no '_' appears before, >>> + # do more checks >>> if char.isupper() and (i > 0) and c_fun_str[i - 1] != '_': >>> if i < length - 1 and c_fun_str[i + 1].islower(): >>> new_name += '_' >> >> The comment paraphrases the if condition. Feels useless. Let's drop >> it. >> >>> @@ -78,8 +79,9 @@ def c_name(name: str, protect: bool = True) -> str: >>> protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int' >>> >>> :param name: The name to map. >>> - :param protect: If true, avoid returning certain ticklish identifiers >>> - (like C keywords) by prepending ``q_``. >>> + :param protect: If true, avoid returning certain ticklish >>> + identifiers (like C keywords) by prepending >>> + ``q_``. >> >> Better: >> >> :param protect: If true, avoid returning certain ticklish >> identifiers (like C keywords) by prepending ``q_``. >> >> For what it's worth, this indentation style is also used in the >> Sphinx-RTD-Tutorial[*]. I like it much better than aligning the text >> like you did, because that wastes screen real estate when the parameter >> names are long, and tempts people to aligning all the parameters, like >> >> :param name: The name to map. >> :param protect: If true, avoid returning certain ticklish identifiers >> (like C keywords) by prepending ``q_``. >> >> which leads to either churn or inconsistency when parameters with longer >> names get added. >> > > Yeah, that should be fine. I don't like the wasted margin space either, > but I suppose I like the "two column" layout for ease of visual > distinction of the parameter names. I suppose it isn't really worth the > kind of column-reformatting churn and the wasted space. > > ...And if we do print a sphinx manual for this, I'll get my visual > distinction there in the rendered output. I'm fine with adopting this > style to cover the entire Python codebase. > > It will be an eventual thing, though: I think we need to agree on a > style guide document and in that same series, fix up the instances of > defying that guide. I think it's important to pair that work, because > the ease of finding and fixing those style deviations will help inform > how pragmatic the style guide is. Makes sense. The introduction of "sphinxy" doc strings (starting with commit adcb9b36c) may have been premature. > I feel like it's something I want to do very soon, but not right now. > Maybe during the next freeze we can tackle it? Whenever you're ready. Until then, I feel we should try to minimize doc string churn. Leave existing doc strings alone unless they're harmful. Add new ones only when we believe they're helpful enough to justify some churn later. >>> """ >>> # ANSI X3J11/88-090, 3.1.1 >>> c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue', >>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py >>> index fee8c671e7..210b56974f 100644 >>> --- a/scripts/qapi/events.py >>> +++ b/scripts/qapi/events.py >>> @@ -48,7 +48,8 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str: >>> """ >>> Generate a struct variable holding the event parameters. >>> >>> - Initialize it with the function arguments defined in `gen_event_send`. >>> + Initialize it with the function arguments defined in >>> + `gen_event_send`. >>> """ >>> assert not typ.variants >>> ret = mcgen(''' >> >> Looks like a wash. I figure the doc string will be rewritten to Sphinx >> format (or whatever other format we adopt for our Python code) anyway, >> so let's not mess with it now. >> >>> @@ -86,9 +87,9 @@ def gen_event_send(name: str, >>> # FIXME: Our declaration of local variables (and of 'errp' in the >>> # parameter list) can collide with exploded members of the event's >>> # data type passed in as parameters. If this collision ever hits in >>> - # practice, we can rename our local variables with a leading _ prefix, >>> - # or split the code into a wrapper function that creates a boxed >>> - # 'param' object then calls another to do the real work. >>> + # practice, we can rename our local variables with a leading _ >>> + # prefix, or split the code into a wrapper function that creates a >>> + # boxed 'param' object then calls another to do the real work. >>> have_args = boxed or (arg_type and not arg_type.is_empty()) >>> >>> ret = mcgen(''' >> >> Improvement. >> >>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >>> index 1fa503bdbd..c54980074e 100644 >>> --- a/scripts/qapi/gen.py >>> +++ b/scripts/qapi/gen.py >>> @@ -63,9 +63,9 @@ def _bottom(self) -> str: >>> return '' >>> >>> def write(self, output_dir: str) -> None: >>> - # Include paths starting with ../ are used to reuse modules of the main >>> - # schema in specialised schemas. Don't overwrite the files that are >>> - # already generated for the main schema. >>> + # Include paths starting with ../ are used to reuse modules >>> + # of the main schema in specialised schemas. Don't overwrite >>> + # the files that are already generated for the main schema. >>> if self.fname.startswith('../'): >>> return >>> pathname = os.path.join(output_dir, self.fname) >> >> Improvement, but mind PEP 8's "You should use two spaces after a >> sentence-ending period in multi-sentence comments". >> > > How important is this, and why? My existing prejudice is that it's only > a superficial detail of writing with no real impact. Holy wars have been fought over less. > (Of course, a single space typist WOULD believe that, wouldn't they? > Those single-space typists are all the same!) I offer three reasons: * Local consistency * Stick to PEP 8 unless you have good reason not to. * It makes Emacs sentence commands work by default. >>> @@ -189,7 +189,7 @@ def _bottom(self) -> str: >>> @contextmanager >>> def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) -> Iterator[None]: >>> """ >>> - A with-statement context manager that wraps with `start_if()` / `end_if()`. >>> + A context manager that wraps output with `start_if()` / `end_if()`. >>> >>> :param ifcond: A sequence of conditionals, passed to `start_if()`. >>> :param args: any number of `QAPIGenCCode`. >> >> Improvement. >> >>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >>> index 9a348ca2e5..faf00013ad 100644 >>> --- a/scripts/qapi/introspect.py >>> +++ b/scripts/qapi/introspect.py >>> @@ -61,8 +61,9 @@ >>> # With optional annotations, the type of all values is: >>> # JSONValue = Union[_Value, Annotated[_Value]] >>> # >>> -# Sadly, mypy does not support recursive types; so the _Stub alias is used to >>> -# mark the imprecision in the type model where we'd otherwise use JSONValue. >>> +# Sadly, mypy does not support recursive types; so the _Stub alias is >>> +# used to mark the imprecision in the type model where we'd otherwise >>> +# use JSONValue. >>> _Stub = Any >>> _Scalar = Union[str, bool, None] >>> _NonScalar = Union[Dict[str, _Stub], List[_Stub]] >> >> Improvement. >> >>> @@ -217,7 +218,8 @@ def visit_end(self) -> None: >>> self._name_map = {} >>> >>> def visit_needed(self, entity: QAPISchemaEntity) -> bool: >>> - # Ignore types on first pass; visit_end() will pick up used types >>> + # Ignore types on first pass; >>> + # visit_end() will pick up used types >> >> Looks a bit odd. Since the original is only slightly over the limit, we >> can keep it. Alternatively. >> >> # Ignore types on first pass; visit_end() will pick up the >> # types that are actually used >> >>> return not isinstance(entity, QAPISchemaType) >>> >>> def _name(self, name: str) -> str: >>> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py >>> index 703e7ed1ed..5bcac83985 100644 >>> --- a/scripts/qapi/main.py >>> +++ b/scripts/qapi/main.py >>> @@ -1,5 +1,5 @@ >>> -# This work is licensed under the terms of the GNU GPL, version 2 or later. >>> -# See the COPYING file in the top-level directory. >>> +# This work is licensed under the terms of the GNU GPL, version 2 or >>> +# later. See the COPYING file in the top-level directory. >> >> Let's drop this one. The line is only slightly too long, and >> consistency with the copright notices elsewhere is more important. >> >>> >>> """ >>> QAPI Generator >>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >>> index 58267c3db9..d5bf91f2b0 100644 >>> --- a/scripts/qapi/parser.py >>> +++ b/scripts/qapi/parser.py >>> @@ -331,8 +331,8 @@ def __init__(self, parser, name=None, indent=0): >>> self._indent = indent >>> >>> def append(self, line): >>> - # Strip leading spaces corresponding to the expected indent level >>> - # Blank lines are always OK. >>> + # Strip leading spaces corresponding to the expected indent >>> + # level. Blank lines are always OK. >>> if line: >>> indent = re.match(r'\s*', line).end() >>> if indent < self._indent: >> >> Improvement, but mind PEP 8's "You should use two spaces after a >> sentence-ending period". >> >>> @@ -353,10 +353,10 @@ def connect(self, member): >>> self.member = member >>> >>> def __init__(self, parser, info): >>> - # self._parser is used to report errors with QAPIParseError. The >>> - # resulting error position depends on the state of the parser. >>> - # It happens to be the beginning of the comment. More or less >>> - # servicable, but action at a distance. >>> + # self._parser is used to report errors with QAPIParseError. >>> + # The resulting error position depends on the state of the >>> + # parser. It happens to be the beginning of the comment. More >>> + # or less servicable, but action at a distance. >>> self._parser = parser >>> self.info = info >>> self.symbol = None >> >> Why not. Two spaces again. >> >>> @@ -430,7 +430,8 @@ def _append_body_line(self, line): >>> if not line.endswith(':'): >>> raise QAPIParseError(self._parser, "line should end with ':'") >>> self.symbol = line[1:-1] >>> - # FIXME invalid names other than the empty string aren't flagged >>> + # FIXME invalid names other than the empty string aren't >>> + # flagged >>> if not self.symbol: >>> raise QAPIParseError(self._parser, "invalid name") >>> elif self.symbol: >> >> Not an improvement, drop the hunk. >> >>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >>> index 703b446fd2..01cdd753cd 100644 >>> --- a/scripts/qapi/schema.py >>> +++ b/scripts/qapi/schema.py >>> @@ -166,9 +166,10 @@ def is_user_module(cls, name: str) -> bool: >>> @classmethod >>> def is_builtin_module(cls, name: str) -> bool: >>> """ >>> - The built-in module is a single System module for the built-in types. >>> + Return true when given the built-in module name. >>> >>> - It is always "./builtin". >>> + The built-in module is a specific System module for the built-in >>> + types. It is always "./builtin". >>> """ >>> return name == cls.BUILTIN_MODULE_NAME >>> >> >> I figure the doc string will be rewritten to Sphinx format anyway, so >> let's not mess with it now. >> >>> @@ -294,7 +295,8 @@ def connect_doc(self, doc=None): >>> m.connect_doc(doc) >>> >>> def is_implicit(self): >>> - # See QAPISchema._make_implicit_enum_type() and ._def_predefineds() >>> + # See QAPISchema._make_implicit_enum_type() and >>> + # ._def_predefineds() >>> return self.name.endswith('Kind') or self.name == 'QType' >>> >>> def c_type(self): >> >> Not an improvement, drop the hunk. >> >>> @@ -421,9 +423,9 @@ def check(self, schema): >>> >>> self.members = members # mark completed >>> >>> - # Check that the members of this type do not cause duplicate JSON members, >>> - # and update seen to track the members seen so far. Report any errors >>> - # on behalf of info, which is not necessarily self.info >>> + # Check that the members of this type do not cause duplicate JSON >>> + # members, and update seen to track the members seen so far. Report >>> + # any errors on behalf of info, which is not necessarily self.info >>> def check_clash(self, info, seen): >>> assert self._checked >>> assert not self.variants # not implemented >> >> Improvement. Two spaces again. >> >>> @@ -494,11 +496,12 @@ def __init__(self, name, info, doc, ifcond, features, variants): >>> def check(self, schema): >>> super().check(schema) >>> self.variants.tag_member.check(schema) >>> - # Not calling self.variants.check_clash(), because there's nothing >>> - # to clash with >>> + # Not calling self.variants.check_clash(), because there's >>> + # nothing to clash with >>> self.variants.check(schema, {}) >>> - # Alternate branch names have no relation to the tag enum values; >>> - # so we have to check for potential name collisions ourselves. >>> + # Alternate branch names have no relation to the tag enum >>> + # values; so we have to check for potential name collisions >>> + # ourselves. >>> seen = {} >>> types_seen = {} >>> for v in self.variants.variants: >> >> Why not. >> >>> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py >>> index 20d572a23a..2e67ab1752 100644 >>> --- a/scripts/qapi/types.py >>> +++ b/scripts/qapi/types.py >>> @@ -35,8 +35,8 @@ >>> from .source import QAPISourceInfo >>> >>> >>> -# variants must be emitted before their container; track what has already >>> -# been output >>> +# variants must be emitted before their container; track what has >>> +# already been output >>> objects_seen = set() >>> >>> >> >> Why not. >> >>> @@ -297,7 +297,8 @@ def _begin_user_module(self, name: str) -> None: >>> ''')) >>> >>> def visit_begin(self, schema: QAPISchema) -> None: >>> - # gen_object() is recursive, ensure it doesn't visit the empty type >>> + # gen_object() is recursive, ensure >>> + # it doesn't visit the empty type >> >> Looks a bit odd. Since the original is only slightly over the limit, we >> can keep it. >> >> Pattern: turning single line comments into multi-line comments to avoid >> small length overruns is usually not an improvement. >> > > Yep, that's my core argument against turning on the option for flake8. > Usually rephrasing is better than re-flowing, but that wasn't always > easy either. (I don't like rewriting things to be less terse, I find it > unpleasant, sorry!) > > Unfortunately, omitting it from flake8 means I'll probably also miss > cases where I or someone else have gone slightly over the limit for > docstrings, and doubt it will be enforced consistently. I'm happy to correct the occasional minor style issue manually. > "Patches welcome" as the old curse goes. > >>> objects_seen.add(schema.the_empty_object_type.name) >>> >>> def _gen_type_cleanup(self, name: str) -> None: >> >> Bottom line: I find some hunks likable enough. >> >> Ways forward: >> >> 1. If you need to respin: >> >> 1.1. you may keep this patch, and work in my feedback. >> >> 1.2. you may drop it. I can pick it up and take care of it. > > This one, please! You got it. > I have to admit that my appetite for consistency runs out right around > here, but I'll never reject someone else doing this kind of work if they > find it important. > > You may also wish to look into the Python packaging series at some > point, as you may be able to augment the tests to provide a "manual" run > that produces some extra warnings from time to time that you may want to > address, which you might find helpful for pursuing these kinds of > cleanups in the future where I suspect they will inevitably regress. Good idea. We may find other warnings we don't want to treat as errors, but do want to consider case by case. >> 2. If we decide to go without a respin: >> >> 2.1. I can work in my feedback in my tree. >> >> 2.2. I can extract the patch and take care of it separately. >> >> I'd prefer to avoid 2.1, because I feel it's too much change for >> comfort. 1.1. vs. 1.2 would be up to you. >> >> >> >> [*] https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html#an-example-class-with-docstrings >>
John Snow <jsnow@redhat.com> writes: > On 4/14/21 11:04 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> > > Thanks for taking this on. I realize it's a slog. > > (Update: much later: AUUUGHHHHH WHY DID I DECIDE TO WRITE DOCS. MY HUBRIS) LOL! >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> scripts/qapi/expr.py | 213 ++++++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 208 insertions(+), 5 deletions(-) >>> >>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >>> index 1869ddf815..adc5b903bc 100644 >>> --- a/scripts/qapi/expr.py >>> +++ b/scripts/qapi/expr.py >>> @@ -1,7 +1,5 @@ >>> # -*- coding: utf-8 -*- >>> # >>> -# Check (context-free) QAPI schema expression structure >>> -# >>> # Copyright IBM, Corp. 2011 >>> # Copyright (c) 2013-2019 Red Hat Inc. >>> # >>> @@ -14,6 +12,25 @@ >>> # This work is licensed under the terms of the GNU GPL, version 2. >>> # See the COPYING file in the top-level directory. >>> >>> +""" >>> +Normalize and validate (context-free) QAPI schema expression structures. >>> + >>> +After QAPI expressions are parsed from disk, they are stored in >>> +recursively nested Python data structures using Dict, List, str, bool, >>> +and int. This module ensures that those nested structures have the >>> +correct type(s) and key(s) where appropriate for the QAPI context-free >>> +grammar. >> >> "from disk"? Perhaps something like "QAPISchemaParser parses the QAPI >> schema into abstract syntax trees consisting of dict, list, str, bool >> and int nodes." This isn't quite accurate; it parses into a list of >> {'expr': AST, 'info': INFO}, but that's detail. >> > > Let's skip the detail; it doesn't help communicate purpose in the first > paragraph here at the high level. The bulk of this module IS primarily > concerned with the structures named. > > Edited to: > > `QAPISchemaParser` parses a QAPI schema into abstract syntax trees > consisting of dict, list, str, bool, and int nodes. This module ensures > that these nested structures have the correct type(s) and key(s) where > appropriate for the QAPI context-free grammar. > > (I replaced "the QAPI schema" with "a QAPI schema" as we have several; > and I wanted to hint at (somehow) that this processes configurable input > (i.e. "from disk") and not something indelibly linked.) > > ((What's wrong with "from disk?")) It can come from anywhere: $ python3 scripts/qapi-gen.py /dev/stdin {'command': 'testme'} >> PEP 8: You should use two spaces after a sentence-ending period in >> multi- sentence comments, except after the final sentence. > > Is this a demand? It's a polite request to save me the (minor) trouble to fix it up in my tree :) >>> + >>> +The QAPI schema expression language allows for syntactic sugar; this >> >> suggest "certain syntactic sugar". >> > > OK > >>> +module also handles the normalization process of these nested >>> +structures. >>> + >>> +See `check_exprs` for the main entry point. >>> + >>> +See `schema.QAPISchema` for processing into native Python data >>> +structures and contextual semantic validation. >>> +""" >>> + >>> import re >>> from typing import ( >>> Collection, >>> @@ -31,9 +48,10 @@ >>> from .source import QAPISourceInfo >>> >>> >>> -# Deserialized JSON objects as returned by the parser; >>> -# The values of this mapping are not necessary to exhaustively type >>> -# here, because the purpose of this module is to interrogate that type. >>> +#: Deserialized JSON objects as returned by the parser. >>> +#: >>> +#: The values of this mapping are not necessary to exhaustively type >>> +#: here, because the purpose of this module is to interrogate that type. >> >> First time I see #: comments; pardon my ignorance. What's the deal? >> > > Sphinx-ese: It indicates that this comment is actually a block relating > to the entity below. It also means that I can cross-reference > `_JSONObject` in docstrings. > > ... which, because of the rewrite where I stopped calling this object an > Expression to avoid overloading a term, is something I actually don't > try to cross-reference anymore. > > So this block can be dropped now, actually. > > (Also: It came up in part one, actually: I snuck one in for EATSPACE, > and reference it in the comment for cgen. We cannot cross-reference > constants unless they are documented, and this is how we accomplish that.) I guess it needs to come up a lot more often for me to actually learn it... Thanks! >>> _JSONObject = Dict[str, object] >>> >>> >>> @@ -48,11 +66,29 @@ >>> def check_name_is_str(name: object, >>> info: QAPISourceInfo, >>> source: str) -> None: >>> + """Ensures that ``name`` is a string.""" >> >> PEP 257: The docstring [...] prescribes the function or method's effect >> as a command ("Do this", "Return that"), not as a description; >> e.g. don't write "Returns the pathname ...". >> >> More of the same below. >> > > Alright. > > While we're here, then ... > > I take this to mean that you prefer: > > :raise: to :raises:, and > :return: to :returns: ? Yes. > And since I need to adjust the verb anyway, I might as well use "Check" > instead of "Ensure". > > """ > > Check that ``name`` is a string. "Check CONDITION" suggests "fail unless CONDITION". "Ensure CONDITION" suggests "do whatever it takes to make CONDITION true, or else fail". The latter gives license to change things, the former doesn't. For instance, when a function is documented to "Check that ``name`` is a string", I expect it to fail when passed a non-string name. Saying "ensure" instead gives it license to convert certain non-string names to string. Do I make sense? > :raise: QAPISemError when ``name`` is an incorrect type. > > """ > > which means our preferred spellings should be: > > :param: (and not parameter, arg, argument, key, keyword) > :raise: (and not raises, except, exception) > :var/ivar/cvar: (variable, instance variable, class variable) > :return: (and not returns) > > Disallow these, as covered by the mypy signature: > > :type: > :vartype: > :rtype: Uh, what happened to "There should be one-- and preferably only one --obvious way to do it"? I'm not disagreeing with anything you wrote. >>> if not isinstance(name, str): >>> raise QAPISemError(info, "%s requires a string name" % source) >>> >>> >>> def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: >>> + """ >>> + Ensures a string is a legal name. >> >> I feel "legal" is best reserved to matters of law. Suggest "valid QAPI >> name". >> >> More of the same below. >> > > Yep, that's the type of review I like here. Getting the terms exactly > correct. You've usually gone through some length to be consistent in > your own writing, but it doesn't always stick with me. > > (I want a jargon file...) > >>> + >>> + A legal name consists of ascii letters, digits, ``-``, and ``_``, >> >> ASCII >> >>> + starting with a letter. The names of downstream extensions are >>> + prefixed with an __com.example_ style prefix, allowing ``.`` and >>> + ``-``. An experimental name is prefixed with ``x-``, following the >>> + RFQDN if present. >> >> I get that "an _com.experimental style prefix" and "the RFQDN" mean the >> same thing, but can we make this clearer? Perhaps >> >> A valid name consists of ascii letters, digits, ``-``, and ``_``, >> starting with a letter. It may be prefixed by a downstream >> prefix of the form __RFQDN_, or the experimental prefix ``x-``. >> If both prefixes are present, the __RFQDN_ prefix goes first. >> > > "It may be prefixed by a downstream prefix" seems redundant, but no > better ideas. Adopted your phrasing wholesale. > >> I'm clueless on proper use of `` in doc strings. Can you educate me? >> > > It's just markup to get "literal" text. In practice, it renders in > monospace. I may not have a great internal barometer for when it should > be used or not. Some tendencies: > > 1. When referring to literal symbols without wanting to invoke any > specific string literal syntax between languages, e.g. 'A' vs "A" might > work well as ``A``. > > 2. When referring to parameter names, to intuit that this is a proper > noun in the code. (The @foo syntax you use in qapi-doc is resolved to > the equivalent of ``foo``when we generate those docs.) > > 3. Generally whenever I need to highlight symbols like ' " ` . _ - that > might get confused for other punctuation, might accidentally render as > markup, or otherwise need some kind of dressing. > > Whenever the noun I want to address is something cross-referencable, I > will instead use `thing` and Sphinx will decorate that reference as it > deems appropriate for the type of symbol that it is. Thanks. I'd expect parameter names to be "cross-referencable" enough from within the function's doc string... >>> + >>> + A legal name cannot start with ``q_``, which is reserved. >>> + >>> + :param name: Name to check. >>> + :param info: QAPI source file information. >> >> Suggest to say "QAPI schema source information", or maybe "QAPI schema >> source file information". >> > > OK > >>> + :param source: Human-readable str describing "what" this name is. >> >> Could mention it's for use in error messages, but we have many similar >> parameters all over the place, so this would perhaps be more tiresome >> than helpful. Fine as is. >> > > Yes, I struggled because I think it doesn't have a consistent "type" of > string that it is -- sometimes it's just the name of the definition > type, sometimes it's a small phrase. Grammatically, I guess it is the > subject of the error sentence. It's whatever the function's error messages want it to be. This is maintainable mostly because we fanatically cover error messages with negative tests in in tests/qapi-schema/. Ensures bad source arguments are quite visible in patches. > If you have a concrete suggestion, I'll take it. Otherwise, I'm just > gonna make it worse. Let's go with what you wrote. >>> + >>> + :return: The stem of the valid name, with no prefixes. >>> + """ >>> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' >>> # and 'q_obj_*' implicit type names. >>> match = valid_name.match(name) >>> @@ -62,6 +98,12 @@ def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: >>> >>> >>> def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: >>> + """ >>> + Ensures a string is a legal event name. >>> + >>> + Checks the same criteria as `check_name_str`, but requires uppercase >>> + and prohibits ``-``. >>> + """ >>> stem = check_name_str(name, info, source) >>> if re.search(r'[a-z-]', stem): >>> raise QAPISemError( >>> @@ -71,6 +113,15 @@ def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: >>> def check_name_lower(name: str, info: QAPISourceInfo, source: str, >>> permit_upper: bool = False, >>> permit_underscore: bool = False) -> None: >>> + """ >>> + Ensures a string is a legal user defined type name. >>> + >>> + Checks the same criteria as `check_name_str`, but may impose >>> + additional constraints. >> >> Correct, but it leads to slightly awkward "permit_FOO: prohibit ... when >> false": >> >>> + >>> + :param permit_upper: Prohibits uppercase when false. >>> + :param permit_underscore: Prohibits underscores when false. >>> + """ >> >> Perhaps something like >> >> Ensure @str is a valid command or member name. >> >> This means it must be a valid QAPI name (as ensured by >> check_name_str()), where the stem consists of lowercase >> characters and '-'. >> >> :param permit_upper: Additionally permit uppercase. >> :param permit_underscore: Additionally permit '_'. >> >> We'd then want to update check_name_upper() and check_name_camel() for >> consistency. >> > """ > Check that ``name`` is a valid user defined type name. > > This means it must be a valid QAPI name as checked by > `check_name_str()`, but where the stem prohibits uppercase > characters and ``_``. > > :param permit_upper: Additionally permit uppercase. > :param permit_underscore: Additionally permit ``_``. > """ Sold. > Using "but where" to highlight the differences, and removing the > parenthetical to make the "see also" more clear to avoid repeating a > paraphrase of what a valid QAPI name is. > > Using literals to highlight @name, because otherwise there is no > processing here that will do the same for us. It may be possible to > extend Sphinx so that it will do it for us, if you are attached to that > visual signifier in the code itself. > >>> stem = check_name_str(name, info, source) >>> if ((not permit_upper and re.search(r'[A-Z]', stem)) >>> or (not permit_underscore and '_' in stem)): >>> @@ -79,12 +130,31 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, >>> >>> >>> def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None: >>> + """ >>> + Ensures a string is a legal CamelCase name. >>> + >>> + Checks the same criteria as `check_name_str`, >>> + but additionally imposes a CamelCase constraint. >>> + """ >>> stem = check_name_str(name, info, source) >>> if not re.match(r'[A-Z][A-Za-z0-9]*[a-z][A-Za-z0-9]*$', stem): >>> raise QAPISemError(info, "name of %s must use CamelCase" % source) >> >> Related: we discussed renaming check_name_{upper,camel,lower} to >> check_name_{event,type,other} or check_name_{event,user_defined_type, >> command_or_member}. >> > > Yep, it's a *little* funny to have check_lower(allow_upper=True) ... but > I am happy with a doc for now. > > """ > > Check that ``name`` is a valid command or member name. > > > > This means it must be a valid QAPI name as checked by > > `check_name_str()`, but where the stem must be in CamelCase. > > """ Good. >>> >>> >>> def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None: >>> + """ >>> + Ensures a name is a legal definition name. >>> + >>> + - 'event' names adhere to `check_name_upper`. >>> + - 'command' names adhere to `check_name_lower`. >>> + - All other names adhere to `check_name_camel`. >> >> When is a name an 'event' name? We should spell out how this uses >> @meta. Perhaps something like: >> >> - If meta == 'event', ... >> - If meta == 'command, ... >> - Else, meta is a type, and ... >> >>> + >>> + All name types must not end with ``Kind`` nor ``List``. >> >> Do you mean "type names"? >> > > I meant "all categories of names". > > "All names must not end with ``Kind`` nor ``List``." > >> Outside this patch's scope: qapi-code-gen.txt reserves suffixes Kind and >> List only for type names, but the code rejects them for events and >> commands, too. One of them should be changed to match the other. Actually, these suffixes get rejected for non-type names before we even get here: in check_name_upper() for event names, and in check_name_lower() for command names. > Instinct is that code should change to match the "spec", as it probably > best represents your intent. Laziness suggests that updating the "spec" > means I don't have to write new tests. The existing negative tests tests/qapi-schema/reserved-type-* both use type names. Adjusting the code doesn't require negative test adjustment. Existing tests do not cover non-type names ending with List or Kind. Covering them requires a positive test if we adjust the code, and a negative test if we adjust the adjust the spec. I doubt covering them is worth the bother. Let's adjust the code and move on. >>> + >>> + :param name: Name to check. >>> + :param info: QAPI source file information. >>> + :param meta: Type name of the QAPI expression. >>> + """ >> >> Glosses over the use of pragma command-name-exceptions. Do we mind? >> > > Mmmm. Nah? I think if you're digging that deep by now you'll have > noticed that check_name_lower() takes some parameters, so the shorter > statement isn't lying. It just isn't telling you exactly how the > parameters are decided. I'm okay with glossing over details where not glossing over them would be a lot of work for little gain, or where it would make the doc strings hard to read for little gain. Maintaining a comparable level of detail in related doc strings is desirable. >>> if meta == 'event': >>> check_name_upper(name, info, meta) >>> elif meta == 'command': >>> @@ -103,6 +173,15 @@ def check_keys(value: _JSONObject, >>> source: str, >>> required: Collection[str], >>> optional: Collection[str]) -> None: >>> + """ >>> + Ensures an object has a specific set of keys. >>> + >>> + :param value: The object to check. >>> + :param info: QAPI source file information. >>> + :param source: Human-readable str describing this value. >>> + :param required: Keys that *must* be present. >>> + :param optional: Keys that *may* be present. >>> + """ >>> > > Style check: Avoid the two-column approach for stuff like this, too? > Also, reminder to self, update the phrasing on ALL :param info: statements. Of the two arguments against the two-column format * waste of screen real estate * churn or inconsistency when parameters with longer names get added the former is moot when none of the descriptions overflows the line. It comes back when we add or edit descriptions that do overflow. So we basically have "churn or inconsistency on certain changes". I accept that more readable doc strings now may be worth a risk of churn later. I wouldn't bother with aligning myself. I think I wouldn't object to aligning until churn gets annoying. >>> def pprint(elems: Iterable[str]) -> str: >>> return ', '.join("'" + e + "'" for e in sorted(elems)) >>> @@ -125,6 +204,12 @@ def pprint(elems: Iterable[str]) -> str: >>> >>> >>> def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> + """ >>> + Ensures common fields in an expression are correct. >> >> Rather vague. The function is really about checking "flag" members, >> i.e. members that must have a boolean value. Perhaps >> >> Ensure flag members (if present) have valid values. >> > > Done! > >>> + >>> + :param expr: Expression to validate. >>> + :param info: QAPI source file information. >>> + """ >>> for key in ['gen', 'success-response']: >>> if key in expr and expr[key] is not False: >>> raise QAPISemError( >>> @@ -143,7 +228,22 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> >>> >>> def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: >>> + """ >>> + Syntactically validate and normalize the ``if`` field of an object. >> >> qapi-code-gen.txt consistently uses "member", not "field". Let's stick >> to "member". >> > > Good, thanks. > >>> >>> + The ``if`` field may be either a ``str`` or a ``List[str]``. >>> + A ``str`` element will be normalized to ``List[str]``. >> >> element? I think you mean value. >> > > Err, yeah. because... it's a single element ... of the list we're gonna > make. Get it? (: > > (Fixed.) > >> Doesn't spell out how str is normalized to List[str], but I guess that's >> obvious enough. >> >>> + >>> + :forms: >>> + :sugared: ``Union[str, List[str]]`` >>> + :canonical: ``List[str]`` >> >> Uh... :param FOO: and :return: are obvious enough, but this :forms: >> stuff feels a bit too fancy for me to rely on voodoo understanding. Can >> you point me to :documentation:? >> > > Haha. > > https://docutils.sourceforge.io/docs/user/rst/quickref.html#field-lists > > The "canonical" field lists we use are just special ones that have been > bookmarked by Sphinx as having special significance. You can use others > arbitrarily, if you want. > > This nests them to achieve a multi-column thing. > > Forms: Sugared: Foo > Canonical: Bar Is :forms: :sugared: ... :canonical: ... your invention? >>> + >>> + :param expr: A ``dict``. >> >> Elsewhere, you have "the object to check", which I like better. >> > > I agree. I was not careful about not accidentally repeating type > information where it wasn't necessary. Semantic descriptions and not > mechanical descriptions are certainly preferred. Fixed! > >>> + The ``if`` field, if present, will be validated. >>> + :param info: QAPI source file information. >>> + >>> + :return: None, ``expr`` is normalized in-place as needed. >>> + """ >>> ifcond = expr.get('if') >>> if ifcond is None: >>> return >>> @@ -167,6 +267,20 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: >>> >>> >>> def normalize_members(members: object) -> None: >>> + """ >>> + Normalize a "members" value. >>> + >>> + If ``members`` is an object, for every value in that object, if that >> >> If it's a dict, actually. >> > > Sigh, yeah. Working at the boundary between two languages is going to > murder what's left of my sanity, I can feel it. > >>> + value is not itself already an object, normalize it to >>> + ``{'type': value}``. >>> + >>> + :forms: >>> + :sugared: ``Dict[str, Union[str, TypeRef]]`` >>> + :canonical: ``Dict[str, TypeRef]`` >>> + >>> + :param members: The members object to normalize. >>> + :return: None, ``members`` is normalized in-place as needed. >>> + """ >>> if isinstance(members, dict): >>> for key, arg in members.items(): >>> if isinstance(arg, dict): >>> @@ -179,6 +293,23 @@ def check_type(value: Optional[object], >>> source: str, >>> allow_array: bool = False, >>> allow_dict: Union[bool, str] = False) -> None: >>> + """ >>> + Check the QAPI type of ``value``. >>> + >>> + Python types of ``str`` or ``None`` are always allowed. >>> + >>> + :param value: The value to check. >>> + :param info: QAPI Source file information. >>> + :param source: Human-readable str describing this value. >>> + :param allow_array: Allow a ``List[str]`` of length 1, >>> + which indicates an Array<T> type. >> >> Leaves open where T comes from. Perhaps "indicates an array of the type >> named by the list element". >> > > Fair. > >>> + :param allow_dict: Allow a dict, treated as an anonymous type. >> >> "treated as anonymous type" isn't quite right. The dict is either >> MEMBERS or BRANCHES in qapi-code-gen.txt parlance. The former is like >> an anonymous struct type, the latter isn't. >> > > Oh, yes. Ehm. > >>> + When given a string, check if that name is >>> + allowed to have keys that use uppercase letters, >>> + and modify validation accordingly. >> >> The second sentence feels both cryptic and vague. >> > > This weird ol' function signature is not done torturing me. > > Maybe: > > :param allow_dict: Allow a dict, which represents a union branch > or an anonymous struct type. This parameter may be given the > struct or union name ``value`` under consideration. In this > case, the name is used to conditionally allow less strict name > validation, based on `QAPISchemaPragma`. > > (Oh, you suggested a fix below. Oops.) > > Argh. Maybe I'll just 'fix' this to have a slightly more laborious > signature. > > def check_type(value: Optional[object], > info: QAPISourceInfo, > source: str, > allow_array: bool = False, > allow_dict: bool = False, > defn_name: str = '') > > and then > > - permissive = False > - if isinstance(allow_dict, str): > - permissive = allow_dict in info.pragma.member_name_exceptions > + permissive = defn_name and defn_name in > info.pragma.member_name_exceptions > > and callers just have to specify both: > > check_type(..., allow_dict=True, defn_name=name) > > and then say: > > :param allow_dict: Allow ``value`` to be a dict, representing a union > branch or an anonymous struct type. > :param defn_name: The struct or union name under consideration. Used to > conditionally allow more permissive member name validation based on > `QAPISchemaPragma`. > > > Stuff for later? Later, please. >>> + >>> + :return: None, ``value`` is normalized in-place as needed. >> >> First mention of normalization. I think we normalize only dicts. >> > > No, I also use that term in the docstrings for this module, check_if, > and normalize_members above. (Maybe your review is non-linear. No problem.) First mention *in this doc string*. In some other doc strings, you mention normalization in the description before you get to :returns:. > I consider desugaring a form of input normalization. I have mentioned it > for :return: to suggest that although there is no return value on the > stack, the value passed (or a descendant thereof) *may* be modified. > > (That is, this isn't "just" a check function, it CAN modify your input!) > > It may occur here because of our call to check_if(). > >> Perhaps: >> >> :param allow_dict: Allow a dict. Its members can be struct type >> members or union branches. When the value of @allow_dict is >> in pragma member-name-exceptions, the dict's keys may violate >> the member naming rules. The dict members are normalized in >> place. >> >> Still neglects to explain we normalize. >> >> Also note the indentation style: it produces reasonable text width >> regardless of parameter name length. Slightly different way to get >> that: >> >> :param allow_dict: >> Allow a dict. Its members can be struct type members or >> union branches. When the value of @allow_dict is in pragma >> member-name-exceptions, the dict's keys may violate the >> member naming rules. The dict members are normalized in >> place. >> > > Oh, I like that style a lot -- it helps preserve the "term / definition" > visual distinction. I may adopt that for any definition longer than a > single-line. > > I'll probably adopt it for this patch and beyond. You're welcome ;) >>> + """ >>> if value is None: >>> return >>> >>> @@ -227,6 +358,21 @@ def check_type(value: Optional[object], >>> >>> def check_features(features: Optional[object], >>> info: QAPISourceInfo) -> None: >>> + """ >>> + Syntactically validate and normalize the ``features`` field. >>> + >>> + ``features`` may be a ``list`` of either ``str`` or ``dict``. >>> + Any ``str`` element will be normalized to ``{'name': element}``. >>> + >>> + :forms: >>> + :sugared: ``List[Union[str, Feature]]`` >>> + :canonical: ``List[Feature]`` >>> + >>> + :param features: an optional list of either str or dict. >>> + :param info: QAPI Source file information. >>> + >>> + :return: None, ``features`` is normalized in-place as needed. >>> + """ >>> if features is None: >>> return >>> if not isinstance(features, list): >>> @@ -244,6 +390,14 @@ def check_features(features: Optional[object], >>> >>> >>> def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> + """ >>> + Validate this expression as an ``enum`` definition. >>> + >>> + :param expr: The expression to validate. >>> + :param info: QAPI source file information. >>> + >>> + :return: None, ``expr`` is normalized in-place as needed. >>> + """ >> >> Unlike the one for check_features(), this one doesn't describe how we >> normalize. More of the same below. Observation, not demand. >> > > I didn't mention specifics because another helper actually does the > work; it's done through descendant calls on fields that may only be > optionally present. > > I tried to keep a consistent phrasing for it. This is another instance of the "how much detail to include" we discussed above. >>> name = expr['enum'] >>> members = expr['data'] >>> prefix = expr.get('prefix') >>> @@ -273,6 +427,14 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> >>> >>> def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> + """ >>> + Validate this expression as a ``struct`` definition. >>> + >>> + :param expr: The expression to validate. >>> + :param info: QAPI source file information. >>> + >>> + :return: None, ``expr`` is normalized in-place as needed. >>> + """ >>> name = cast(str, expr['struct']) # Asserted in check_exprs >>> members = expr['data'] >>> >>> @@ -281,6 +443,14 @@ def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> >>> >>> def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> + """ >>> + Validate this expression as a ``union`` definition. >>> + >>> + :param expr: The expression to validate. >>> + :param info: QAPI source file information. >>> + >>> + :return: None, ``expr`` is normalized in-place as needed. >>> + """ >>> name = cast(str, expr['union']) # Asserted in check_exprs >>> base = expr.get('base') >>> discriminator = expr.get('discriminator') >>> @@ -309,6 +479,14 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> >>> >>> def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> + """ >>> + Validate this expression as an ``alternate`` definition. >>> + >>> + :param expr: The expression to validate. >>> + :param info: QAPI source file information. >>> + >>> + :return: None, ``expr`` is normalized in-place as needed. >>> + """ >>> members = expr['data'] >>> >>> if not members: >>> @@ -326,6 +504,14 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> >>> >>> def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> + """ >>> + Validate this expression as a ``command`` definition. >>> + >>> + :param expr: The expression to validate. >>> + :param info: QAPI source file information. >>> + >>> + :return: None, ``expr`` is normalized in-place as needed. >>> + """ >>> args = expr.get('data') >>> rets = expr.get('returns') >>> boxed = expr.get('boxed', False) >>> @@ -337,6 +523,14 @@ def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> >>> >>> def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> + """ >>> + Normalize and validate this expression as an ``event`` definition. >>> + >>> + :param expr: The expression to validate. >>> + :param info: QAPI source file information. >>> + >>> + :return: None, ``expr`` is normalized in-place as needed. >>> + """ >>> args = expr.get('data') >>> boxed = expr.get('boxed', False) >>> >>> @@ -346,6 +540,15 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: >>> >>> >>> def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: >>> + """ >>> + Validate and normalize a list of parsed QAPI schema expressions. >>> + >>> + This function accepts a list of expressions + metadta as returned by >> >> Typo: metadata. >> > > I've invented a new kind of data, actually. > > (Fixed.) > >>> + the parser. It destructively normalizes the expressions in-place. >>> + >>> + :param exprs: The list of expressions to normalize/validate. >>> + :return: The same list of expressions (now modified). >>> + """ >>> for expr_elem in exprs: >>> # Expression >>> assert isinstance(expr_elem['expr'], dict) > > Made a bunch of changes, but held off on trying to "finish" it; I want > to make a checklist for myself to review with your counter-feedback and > methodically revise it all in one shot. Makes sense. > Thanks! Glad the effort is of some use!
On 4/17/21 6:52 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: >> On 4/16/21 8:44 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >> It will be an eventual thing, though: I think we need to agree on a >> style guide document and in that same series, fix up the instances of >> defying that guide. I think it's important to pair that work, because >> the ease of finding and fixing those style deviations will help inform >> how pragmatic the style guide is. > > Makes sense. > > The introduction of "sphinxy" doc strings (starting with commit > adcb9b36c) may have been premature. > Somewhat premature, but what other format is there? It would have been worse to adopt Numpy or google style. We'll dial it in over time, it will be fine. >> I feel like it's something I want to do very soon, but not right now. >> Maybe during the next freeze we can tackle it? > > Whenever you're ready. > > Until then, I feel we should try to minimize doc string churn. Leave > existing doc strings alone unless they're harmful. Add new ones only > when we believe they're helpful enough to justify some churn later. > OK. After the expr comments, I actually didn't add very many. I think I add one or two for the parser because I had trouble understanding at a glance how it worked, but most of the tiny functions and helpers I left alone. I barely touched schema.py, because it was complex and I had some visions of refactoring it a little to make some of the typing better later. >>> Improvement, but mind PEP 8's "You should use two spaces after a >>> sentence-ending period in multi-sentence comments". >>> >> >> How important is this, and why? My existing prejudice is that it's only >> a superficial detail of writing with no real impact. > > Holy wars have been fought over less. > :) >> (Of course, a single space typist WOULD believe that, wouldn't they? >> Those single-space typists are all the same!) > > I offer three reasons: > > * Local consistency > > * Stick to PEP 8 unless you have good reason not to. > > * It makes Emacs sentence commands work by default. > For me, it's another thing in the category of "I don't actually mind either way", and can foresee myself accepting a patch using either style without comment. Inconsistency here doesn't really bother me unless it's inconsistent within a single docstring. For QAPI, since you're the maintainer, I can adhere to your style. For the purposes of all Python code, though, I am not sure I want to bother enforcing it myself. You're always welcome to post-edit anything I've written for typographical consistency as you see fit, I genuinely won't mind. (It saves me the trouble of having to copy-edit for something I am visually blind to.) That said, I'll try to match your preferred style for QAPI at a minimum. I notice that emacs' reflow command does not always insert two spaces if a paragraph already sneaks in under the column limit; is there a way to *force* it to add two spaces? >> Unfortunately, omitting it from flake8 means I'll probably also miss >> cases where I or someone else have gone slightly over the limit for >> docstrings, and doubt it will be enforced consistently. > > I'm happy to correct the occasional minor style issue manually. > If you accept that burden then I have no leg to stand on, I suppose :) >>> 1.2. you may drop it. I can pick it up and take care of it. >> >> This one, please! > > You got it. > Thanks! You can do that whenever, it won't interfere with anything in the interim. --js
On 4/17/21 9:18 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 4/14/21 11:04 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >>> >> >> Thanks for taking this on. I realize it's a slog. >> >> (Update: much later: AUUUGHHHHH WHY DID I DECIDE TO WRITE DOCS. MY HUBRIS) > > LOL! > >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> scripts/qapi/expr.py | 213 ++++++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 208 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >>>> index 1869ddf815..adc5b903bc 100644 >>>> --- a/scripts/qapi/expr.py >>>> +++ b/scripts/qapi/expr.py >>>> @@ -1,7 +1,5 @@ >>>> # -*- coding: utf-8 -*- >>>> # >>>> -# Check (context-free) QAPI schema expression structure >>>> -# >>>> # Copyright IBM, Corp. 2011 >>>> # Copyright (c) 2013-2019 Red Hat Inc. >>>> # >>>> @@ -14,6 +12,25 @@ >>>> # This work is licensed under the terms of the GNU GPL, version 2. >>>> # See the COPYING file in the top-level directory. >>>> >>>> +""" >>>> +Normalize and validate (context-free) QAPI schema expression structures. >>>> + >>>> +After QAPI expressions are parsed from disk, they are stored in >>>> +recursively nested Python data structures using Dict, List, str, bool, >>>> +and int. This module ensures that those nested structures have the >>>> +correct type(s) and key(s) where appropriate for the QAPI context-free >>>> +grammar. >>> >>> "from disk"? Perhaps something like "QAPISchemaParser parses the QAPI >>> schema into abstract syntax trees consisting of dict, list, str, bool >>> and int nodes." This isn't quite accurate; it parses into a list of >>> {'expr': AST, 'info': INFO}, but that's detail. >>> >> >> Let's skip the detail; it doesn't help communicate purpose in the first >> paragraph here at the high level. The bulk of this module IS primarily >> concerned with the structures named. >> >> Edited to: >> >> `QAPISchemaParser` parses a QAPI schema into abstract syntax trees >> consisting of dict, list, str, bool, and int nodes. This module ensures >> that these nested structures have the correct type(s) and key(s) where >> appropriate for the QAPI context-free grammar. >> >> (I replaced "the QAPI schema" with "a QAPI schema" as we have several; >> and I wanted to hint at (somehow) that this processes configurable input >> (i.e. "from disk") and not something indelibly linked.) >> >> ((What's wrong with "from disk?")) > > It can come from anywhere: > > $ python3 scripts/qapi-gen.py /dev/stdin > {'command': 'testme'} > >>> PEP 8: You should use two spaces after a sentence-ending period in >>> multi- sentence comments, except after the final sentence. >> >> Is this a demand? > > It's a polite request to save me the (minor) trouble to fix it up in my > tree :) > :( (I took a stab at it. May have missed a spot or two...) >>>> + >>>> +The QAPI schema expression language allows for syntactic sugar; this >>> >>> suggest "certain syntactic sugar". >>> >> >> OK >> >>>> +module also handles the normalization process of these nested >>>> +structures. >>>> + >>>> +See `check_exprs` for the main entry point. >>>> + >>>> +See `schema.QAPISchema` for processing into native Python data >>>> +structures and contextual semantic validation. >>>> +""" >>>> + >>>> import re >>>> from typing import ( >>>> Collection, >>>> @@ -31,9 +48,10 @@ >>>> from .source import QAPISourceInfo >>>> >>>> >>>> -# Deserialized JSON objects as returned by the parser; >>>> -# The values of this mapping are not necessary to exhaustively type >>>> -# here, because the purpose of this module is to interrogate that type. >>>> +#: Deserialized JSON objects as returned by the parser. >>>> +#: >>>> +#: The values of this mapping are not necessary to exhaustively type >>>> +#: here, because the purpose of this module is to interrogate that type. >>> >>> First time I see #: comments; pardon my ignorance. What's the deal? >>> >> >> Sphinx-ese: It indicates that this comment is actually a block relating >> to the entity below. It also means that I can cross-reference >> `_JSONObject` in docstrings. >> >> ... which, because of the rewrite where I stopped calling this object an >> Expression to avoid overloading a term, is something I actually don't >> try to cross-reference anymore. >> >> So this block can be dropped now, actually. >> >> (Also: It came up in part one, actually: I snuck one in for EATSPACE, >> and reference it in the comment for cgen. We cannot cross-reference >> constants unless they are documented, and this is how we accomplish that.) > > I guess it needs to come up a lot more often for me to actually learn > it... Thanks! > I am in love with the idea of a project-wide cross reference system, but it's been tough to get the right ideas in my head for how to do it. >>>> _JSONObject = Dict[str, object] >>>> >>>> >>>> @@ -48,11 +66,29 @@ >>>> def check_name_is_str(name: object, >>>> info: QAPISourceInfo, >>>> source: str) -> None: >>>> + """Ensures that ``name`` is a string.""" >>> >>> PEP 257: The docstring [...] prescribes the function or method's effect >>> as a command ("Do this", "Return that"), not as a description; >>> e.g. don't write "Returns the pathname ...". >>> >>> More of the same below. >>> >> >> Alright. >> >> While we're here, then ... >> >> I take this to mean that you prefer: >> >> :raise: to :raises:, and >> :return: to :returns: ? > > Yes. > >> And since I need to adjust the verb anyway, I might as well use "Check" >> instead of "Ensure". >> >> """ >> >> Check that ``name`` is a string. > > "Check CONDITION" suggests "fail unless CONDITION". > > "Ensure CONDITION" suggests "do whatever it takes to make CONDITION > true, or else fail". > > The latter gives license to change things, the former doesn't. > > For instance, when a function is documented to "Check that ``name`` is a > string", I expect it to fail when passed a non-string name. Saying > "ensure" instead gives it license to convert certain non-string names to > string. > > Do I make sense? > Sure, if that's your preference. I've reverted that change. >> :raise: QAPISemError when ``name`` is an incorrect type. >> >> """ >> >> which means our preferred spellings should be: >> >> :param: (and not parameter, arg, argument, key, keyword) >> :raise: (and not raises, except, exception) >> :var/ivar/cvar: (variable, instance variable, class variable) >> :return: (and not returns) >> >> Disallow these, as covered by the mypy signature: >> >> :type: >> :vartype: >> :rtype: > > Uh, what happened to "There should be one-- and preferably only one > --obvious way to do it"? > > I'm not disagreeing with anything you wrote. > Guido had nothing to do with Sphinx. Sadly, the "we don't stipulate what has to be here" means there's no real ... standard. Up to us to make our own. :\ This is the sort of thing that led to putting type hints directly in the Python standard. Maybe one day we can help impose a "canonical" sphinx-ese docstring dialect. >>>> if not isinstance(name, str): >>>> raise QAPISemError(info, "%s requires a string name" % source) >>>> >>>> >>>> def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: >>>> + """ >>>> + Ensures a string is a legal name. >>> >>> I feel "legal" is best reserved to matters of law. Suggest "valid QAPI >>> name". >>> >>> More of the same below. >>> >> >> Yep, that's the type of review I like here. Getting the terms exactly >> correct. You've usually gone through some length to be consistent in >> your own writing, but it doesn't always stick with me. >> >> (I want a jargon file...) >> >>>> + >>>> + A legal name consists of ascii letters, digits, ``-``, and ``_``, >>> >>> ASCII >>> >>>> + starting with a letter. The names of downstream extensions are >>>> + prefixed with an __com.example_ style prefix, allowing ``.`` and >>>> + ``-``. An experimental name is prefixed with ``x-``, following the >>>> + RFQDN if present. >>> >>> I get that "an _com.experimental style prefix" and "the RFQDN" mean the >>> same thing, but can we make this clearer? Perhaps >>> >>> A valid name consists of ascii letters, digits, ``-``, and ``_``, >>> starting with a letter. It may be prefixed by a downstream >>> prefix of the form __RFQDN_, or the experimental prefix ``x-``. >>> If both prefixes are present, the __RFQDN_ prefix goes first. >>> >> >> "It may be prefixed by a downstream prefix" seems redundant, but no >> better ideas. Adopted your phrasing wholesale. >> >>> I'm clueless on proper use of `` in doc strings. Can you educate me? >>> >> >> It's just markup to get "literal" text. In practice, it renders in >> monospace. I may not have a great internal barometer for when it should >> be used or not. Some tendencies: >> >> 1. When referring to literal symbols without wanting to invoke any >> specific string literal syntax between languages, e.g. 'A' vs "A" might >> work well as ``A``. >> >> 2. When referring to parameter names, to intuit that this is a proper >> noun in the code. (The @foo syntax you use in qapi-doc is resolved to >> the equivalent of ``foo``when we generate those docs.) >> >> 3. Generally whenever I need to highlight symbols like ' " ` . _ - that >> might get confused for other punctuation, might accidentally render as >> markup, or otherwise need some kind of dressing. >> >> Whenever the noun I want to address is something cross-referencable, I >> will instead use `thing` and Sphinx will decorate that reference as it >> deems appropriate for the type of symbol that it is. > > Thanks. > > I'd expect parameter names to be "cross-referencable" enough from within > the function's doc string... > Yeah, I read through the Sphinx code for Python recently while prototyping a turbocharged QAPI domain and it looked like they were theoretically reference-able, but in practice it didn't seem like they were. I have to look into it more, but for right now I don't think they are ... I suspect I will find out more as I continue to develop prototypes for the QMP cross-referencing. >>>> + >>>> + A legal name cannot start with ``q_``, which is reserved. >>>> + >>>> + :param name: Name to check. >>>> + :param info: QAPI source file information. >>> >>> Suggest to say "QAPI schema source information", or maybe "QAPI schema >>> source file information". >>> >> >> OK >> >>>> + :param source: Human-readable str describing "what" this name is. >>> >>> Could mention it's for use in error messages, but we have many similar >>> parameters all over the place, so this would perhaps be more tiresome >>> than helpful. Fine as is. >>> >> >> Yes, I struggled because I think it doesn't have a consistent "type" of >> string that it is -- sometimes it's just the name of the definition >> type, sometimes it's a small phrase. Grammatically, I guess it is the >> subject of the error sentence. > > It's whatever the function's error messages want it to be. > > This is maintainable mostly because we fanatically cover error messages > with negative tests in in tests/qapi-schema/. Ensures bad source > arguments are quite visible in patches. > >> If you have a concrete suggestion, I'll take it. Otherwise, I'm just >> gonna make it worse. > > Let's go with what you wrote. > Couldn't help myself and I fiddled with it. >>>> + >>>> + :return: The stem of the valid name, with no prefixes. >>>> + """ >>>> # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' >>>> # and 'q_obj_*' implicit type names. >>>> match = valid_name.match(name) >>>> @@ -62,6 +98,12 @@ def check_name_str(name: str, info: QAPISourceInfo, source: str) -> str: >>>> >>>> >>>> def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: >>>> + """ >>>> + Ensures a string is a legal event name. >>>> + >>>> + Checks the same criteria as `check_name_str`, but requires uppercase >>>> + and prohibits ``-``. >>>> + """ >>>> stem = check_name_str(name, info, source) >>>> if re.search(r'[a-z-]', stem): >>>> raise QAPISemError( >>>> @@ -71,6 +113,15 @@ def check_name_upper(name: str, info: QAPISourceInfo, source: str) -> None: >>>> def check_name_lower(name: str, info: QAPISourceInfo, source: str, >>>> permit_upper: bool = False, >>>> permit_underscore: bool = False) -> None: >>>> + """ >>>> + Ensures a string is a legal user defined type name. >>>> + >>>> + Checks the same criteria as `check_name_str`, but may impose >>>> + additional constraints. >>> >>> Correct, but it leads to slightly awkward "permit_FOO: prohibit ... when >>> false": >>> >>>> + >>>> + :param permit_upper: Prohibits uppercase when false. >>>> + :param permit_underscore: Prohibits underscores when false. >>>> + """ >>> >>> Perhaps something like >>> >>> Ensure @str is a valid command or member name. >>> >>> This means it must be a valid QAPI name (as ensured by >>> check_name_str()), where the stem consists of lowercase >>> characters and '-'. >>> >>> :param permit_upper: Additionally permit uppercase. >>> :param permit_underscore: Additionally permit '_'. >>> >>> We'd then want to update check_name_upper() and check_name_camel() for >>> consistency. >>> >> """ >> Check that ``name`` is a valid user defined type name. >> >> This means it must be a valid QAPI name as checked by >> `check_name_str()`, but where the stem prohibits uppercase >> characters and ``_``. >> >> :param permit_upper: Additionally permit uppercase. >> :param permit_underscore: Additionally permit ``_``. >> """ > > Sold. > >> Using "but where" to highlight the differences, and removing the >> parenthetical to make the "see also" more clear to avoid repeating a >> paraphrase of what a valid QAPI name is. >> >> Using literals to highlight @name, because otherwise there is no >> processing here that will do the same for us. It may be possible to >> extend Sphinx so that it will do it for us, if you are attached to that >> visual signifier in the code itself. >> >>>> stem = check_name_str(name, info, source) >>>> if ((not permit_upper and re.search(r'[A-Z]', stem)) >>>> or (not permit_underscore and '_' in stem)): >>>> @@ -79,12 +130,31 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, >>>> >>>> >>>> def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None: >>>> + """ >>>> + Ensures a string is a legal CamelCase name. >>>> + >>>> + Checks the same criteria as `check_name_str`, >>>> + but additionally imposes a CamelCase constraint. >>>> + """ >>>> stem = check_name_str(name, info, source) >>>> if not re.match(r'[A-Z][A-Za-z0-9]*[a-z][A-Za-z0-9]*$', stem): >>>> raise QAPISemError(info, "name of %s must use CamelCase" % source) >>> >>> Related: we discussed renaming check_name_{upper,camel,lower} to >>> check_name_{event,type,other} or check_name_{event,user_defined_type, >>> command_or_member}. >>> >> >> Yep, it's a *little* funny to have check_lower(allow_upper=True) ... but >> I am happy with a doc for now. >> >> """ >> >> Check that ``name`` is a valid command or member name. >> >> >> >> This means it must be a valid QAPI name as checked by >> >> `check_name_str()`, but where the stem must be in CamelCase. >> >> """ > > Good. > (Oops, copy-pasting from emacs buffer copied literal spaces again. Long standing bug I have with my emacs configuration. It likes to copy spaces following comment tokens sometimes.) >>>> >>>> >>>> def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None: >>>> + """ >>>> + Ensures a name is a legal definition name. >>>> + >>>> + - 'event' names adhere to `check_name_upper`. >>>> + - 'command' names adhere to `check_name_lower`. >>>> + - All other names adhere to `check_name_camel`. >>> >>> When is a name an 'event' name? We should spell out how this uses >>> @meta. Perhaps something like: >>> >>> - If meta == 'event', ... >>> - If meta == 'command, ... >>> - Else, meta is a type, and ... >>> >>>> + >>>> + All name types must not end with ``Kind`` nor ``List``. >>> >>> Do you mean "type names"? >>> >> >> I meant "all categories of names". >> >> "All names must not end with ``Kind`` nor ``List``." >> >>> Outside this patch's scope: qapi-code-gen.txt reserves suffixes Kind and >>> List only for type names, but the code rejects them for events and >>> commands, too. One of them should be changed to match the other. > > Actually, these suffixes get rejected for non-type names before we even > get here: in check_name_upper() for event names, and in > check_name_lower() for command names. > >> Instinct is that code should change to match the "spec", as it probably >> best represents your intent. Laziness suggests that updating the "spec" >> means I don't have to write new tests. > > The existing negative tests tests/qapi-schema/reserved-type-* both use > type names. Adjusting the code doesn't require negative test > adjustment. > > Existing tests do not cover non-type names ending with List or Kind. > Covering them requires a positive test if we adjust the code, and a > negative test if we adjust the adjust the spec. I doubt covering them > is worth the bother. > > Let's adjust the code and move on. > OK, I can add a new patch just before this one. I may have misunderstood your goal, but you can take hold of the wheel if needs be. >>>> + >>>> + :param name: Name to check. >>>> + :param info: QAPI source file information. >>>> + :param meta: Type name of the QAPI expression. >>>> + """ >>> >>> Glosses over the use of pragma command-name-exceptions. Do we mind? >>> >> >> Mmmm. Nah? I think if you're digging that deep by now you'll have >> noticed that check_name_lower() takes some parameters, so the shorter >> statement isn't lying. It just isn't telling you exactly how the >> parameters are decided. > > I'm okay with glossing over details where not glossing over them would > be a lot of work for little gain, or where it would make the doc strings > hard to read for little gain. > > Maintaining a comparable level of detail in related doc strings is > desirable. > Not sure if that means you're OK with the omission or not. I'll leave it as-is for now, then. >>>> if meta == 'event': >>>> check_name_upper(name, info, meta) >>>> elif meta == 'command': >>>> @@ -103,6 +173,15 @@ def check_keys(value: _JSONObject, >>>> source: str, >>>> required: Collection[str], >>>> optional: Collection[str]) -> None: >>>> + """ >>>> + Ensures an object has a specific set of keys. >>>> + >>>> + :param value: The object to check. >>>> + :param info: QAPI source file information. >>>> + :param source: Human-readable str describing this value. >>>> + :param required: Keys that *must* be present. >>>> + :param optional: Keys that *may* be present. >>>> + """ >>>> >> >> Style check: Avoid the two-column approach for stuff like this, too? >> Also, reminder to self, update the phrasing on ALL :param info: statements. > > Of the two arguments against the two-column format > > * waste of screen real estate > > * churn or inconsistency when parameters with longer names get added > > the former is moot when none of the descriptions overflows the line. It > comes back when we add or edit descriptions that do overflow. So we > basically have "churn or inconsistency on certain changes". > > I accept that more readable doc strings now may be worth a risk of churn > later. > > I wouldn't bother with aligning myself. I think I wouldn't object to > aligning until churn gets annoying. > I made some edits to remove the two column format to try and be consistent. I used the new indent formatting in a few places where it seemed appropriate. >>>> def pprint(elems: Iterable[str]) -> str: >>>> return ', '.join("'" + e + "'" for e in sorted(elems)) >>>> @@ -125,6 +204,12 @@ def pprint(elems: Iterable[str]) -> str: >>>> >>>> >>>> def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> + """ >>>> + Ensures common fields in an expression are correct. >>> >>> Rather vague. The function is really about checking "flag" members, >>> i.e. members that must have a boolean value. Perhaps >>> >>> Ensure flag members (if present) have valid values. >>> >> >> Done! >> >>>> + >>>> + :param expr: Expression to validate. >>>> + :param info: QAPI source file information. >>>> + """ >>>> for key in ['gen', 'success-response']: >>>> if key in expr and expr[key] is not False: >>>> raise QAPISemError( >>>> @@ -143,7 +228,22 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> >>>> >>>> def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: >>>> + """ >>>> + Syntactically validate and normalize the ``if`` field of an object. >>> >>> qapi-code-gen.txt consistently uses "member", not "field". Let's stick >>> to "member". >>> >> >> Good, thanks. >> >>>> >>>> + The ``if`` field may be either a ``str`` or a ``List[str]``. >>>> + A ``str`` element will be normalized to ``List[str]``. >>> >>> element? I think you mean value. >>> >> >> Err, yeah. because... it's a single element ... of the list we're gonna >> make. Get it? (: >> >> (Fixed.) >> >>> Doesn't spell out how str is normalized to List[str], but I guess that's >>> obvious enough. >>> >>>> + >>>> + :forms: >>>> + :sugared: ``Union[str, List[str]]`` >>>> + :canonical: ``List[str]`` >>> >>> Uh... :param FOO: and :return: are obvious enough, but this :forms: >>> stuff feels a bit too fancy for me to rely on voodoo understanding. Can >>> you point me to :documentation:? >>> >> >> Haha. >> >> https://docutils.sourceforge.io/docs/user/rst/quickref.html#field-lists >> >> The "canonical" field lists we use are just special ones that have been >> bookmarked by Sphinx as having special significance. You can use others >> arbitrarily, if you want. >> >> This nests them to achieve a multi-column thing. >> >> Forms: Sugared: Foo >> Canonical: Bar > > Is :forms: :sugared: ... :canonical: ... your invention? > In the sense that those words do not mean anything to Sphinx, that's correct. It's just ReST markup that cooperates with the other field lists for tidy rendered output. e.g. Parameters: A B C Forms: Sugared: Foo Canonical: Bar i.e. it's precisely as arbitrary as: * Forms: - Sugared: Foo - Canonical: Bar What I have learned is that Sphinx suggests certain field list names for you to use (param, params, arg, etc) and will do special book-keeping for those, but you can use anything you want. For example, using :raise TYPE: will adjust the rendering slightly to add cross-references to the type name mentioned for you -- they are specially processed. It's good to always use the ones Sphinx knows about for parameters and so on, because that's where you'd add hooks to change the rendering format for those, it's how things like autodoc does generated documentation, etc. In this case, I wanted to briefly document what the forms were expected to look like while reading this function so I had some basis for quickly remembering what the data shape was, because my memory for those details is poor. I chose a field list to represent this information for visual parity with the other "input/output" descriptions. >>>> + >>>> + :param expr: A ``dict``. >>> >>> Elsewhere, you have "the object to check", which I like better. >>> >> >> I agree. I was not careful about not accidentally repeating type >> information where it wasn't necessary. Semantic descriptions and not >> mechanical descriptions are certainly preferred. Fixed! >> >>>> + The ``if`` field, if present, will be validated. >>>> + :param info: QAPI source file information. >>>> + >>>> + :return: None, ``expr`` is normalized in-place as needed. >>>> + """ >>>> ifcond = expr.get('if') >>>> if ifcond is None: >>>> return >>>> @@ -167,6 +267,20 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: >>>> >>>> >>>> def normalize_members(members: object) -> None: >>>> + """ >>>> + Normalize a "members" value. >>>> + >>>> + If ``members`` is an object, for every value in that object, if that >>> >>> If it's a dict, actually. >>> >> >> Sigh, yeah. Working at the boundary between two languages is going to >> murder what's left of my sanity, I can feel it. >> >>>> + value is not itself already an object, normalize it to >>>> + ``{'type': value}``. >>>> + >>>> + :forms: >>>> + :sugared: ``Dict[str, Union[str, TypeRef]]`` >>>> + :canonical: ``Dict[str, TypeRef]`` >>>> + >>>> + :param members: The members object to normalize. >>>> + :return: None, ``members`` is normalized in-place as needed. >>>> + """ >>>> if isinstance(members, dict): >>>> for key, arg in members.items(): >>>> if isinstance(arg, dict): >>>> @@ -179,6 +293,23 @@ def check_type(value: Optional[object], >>>> source: str, >>>> allow_array: bool = False, >>>> allow_dict: Union[bool, str] = False) -> None: >>>> + """ >>>> + Check the QAPI type of ``value``. >>>> + >>>> + Python types of ``str`` or ``None`` are always allowed. >>>> + >>>> + :param value: The value to check. >>>> + :param info: QAPI Source file information. >>>> + :param source: Human-readable str describing this value. >>>> + :param allow_array: Allow a ``List[str]`` of length 1, >>>> + which indicates an Array<T> type. >>> >>> Leaves open where T comes from. Perhaps "indicates an array of the type >>> named by the list element". >>> >> >> Fair. >> >>>> + :param allow_dict: Allow a dict, treated as an anonymous type. >>> >>> "treated as anonymous type" isn't quite right. The dict is either >>> MEMBERS or BRANCHES in qapi-code-gen.txt parlance. The former is like >>> an anonymous struct type, the latter isn't. >>> >> >> Oh, yes. Ehm. >> >>>> + When given a string, check if that name is >>>> + allowed to have keys that use uppercase letters, >>>> + and modify validation accordingly. >>> >>> The second sentence feels both cryptic and vague. >>> >> >> This weird ol' function signature is not done torturing me. >> >> Maybe: >> >> :param allow_dict: Allow a dict, which represents a union branch >> or an anonymous struct type. This parameter may be given the >> struct or union name ``value`` under consideration. In this >> case, the name is used to conditionally allow less strict name >> validation, based on `QAPISchemaPragma`. >> >> (Oh, you suggested a fix below. Oops.) >> >> Argh. Maybe I'll just 'fix' this to have a slightly more laborious >> signature. >> >> def check_type(value: Optional[object], >> info: QAPISourceInfo, >> source: str, >> allow_array: bool = False, >> allow_dict: bool = False, >> defn_name: str = '') >> >> and then >> >> - permissive = False >> - if isinstance(allow_dict, str): >> - permissive = allow_dict in info.pragma.member_name_exceptions >> + permissive = defn_name and defn_name in >> info.pragma.member_name_exceptions >> >> and callers just have to specify both: >> >> check_type(..., allow_dict=True, defn_name=name) >> >> and then say: >> >> :param allow_dict: Allow ``value`` to be a dict, representing a union >> branch or an anonymous struct type. >> :param defn_name: The struct or union name under consideration. Used to >> conditionally allow more permissive member name validation based on >> `QAPISchemaPragma`. >> >> >> Stuff for later? > > Later, please. > >>>> + >>>> + :return: None, ``value`` is normalized in-place as needed. >>> >>> First mention of normalization. I think we normalize only dicts. >>> >> >> No, I also use that term in the docstrings for this module, check_if, >> and normalize_members above. (Maybe your review is non-linear. No problem.) > > First mention *in this doc string*. In some other doc strings, you > mention normalization in the description before you get to :returns:. > Oh, I see. I made some edits for clarity. >> I consider desugaring a form of input normalization. I have mentioned it >> for :return: to suggest that although there is no return value on the >> stack, the value passed (or a descendant thereof) *may* be modified. >> >> (That is, this isn't "just" a check function, it CAN modify your input!) >> >> It may occur here because of our call to check_if(). >> >>> Perhaps: >>> >>> :param allow_dict: Allow a dict. Its members can be struct type >>> members or union branches. When the value of @allow_dict is >>> in pragma member-name-exceptions, the dict's keys may violate >>> the member naming rules. The dict members are normalized in >>> place. >>> >>> Still neglects to explain we normalize. >>> >>> Also note the indentation style: it produces reasonable text width >>> regardless of parameter name length. Slightly different way to get >>> that: >>> >>> :param allow_dict: >>> Allow a dict. Its members can be struct type members or >>> union branches. When the value of @allow_dict is in pragma >>> member-name-exceptions, the dict's keys may violate the >>> member naming rules. The dict members are normalized in >>> place. >>> >> >> Oh, I like that style a lot -- it helps preserve the "term / definition" >> visual distinction. I may adopt that for any definition longer than a >> single-line. >> >> I'll probably adopt it for this patch and beyond. > > You're welcome ;) > >>>> + """ >>>> if value is None: >>>> return >>>> >>>> @@ -227,6 +358,21 @@ def check_type(value: Optional[object], >>>> >>>> def check_features(features: Optional[object], >>>> info: QAPISourceInfo) -> None: >>>> + """ >>>> + Syntactically validate and normalize the ``features`` field. >>>> + >>>> + ``features`` may be a ``list`` of either ``str`` or ``dict``. >>>> + Any ``str`` element will be normalized to ``{'name': element}``. >>>> + >>>> + :forms: >>>> + :sugared: ``List[Union[str, Feature]]`` >>>> + :canonical: ``List[Feature]`` >>>> + >>>> + :param features: an optional list of either str or dict. >>>> + :param info: QAPI Source file information. >>>> + >>>> + :return: None, ``features`` is normalized in-place as needed. >>>> + """ >>>> if features is None: >>>> return >>>> if not isinstance(features, list): >>>> @@ -244,6 +390,14 @@ def check_features(features: Optional[object], >>>> >>>> >>>> def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> + """ >>>> + Validate this expression as an ``enum`` definition. >>>> + >>>> + :param expr: The expression to validate. >>>> + :param info: QAPI source file information. >>>> + >>>> + :return: None, ``expr`` is normalized in-place as needed. >>>> + """ >>> >>> Unlike the one for check_features(), this one doesn't describe how we >>> normalize. More of the same below. Observation, not demand. >>> >> >> I didn't mention specifics because another helper actually does the >> work; it's done through descendant calls on fields that may only be >> optionally present. >> >> I tried to keep a consistent phrasing for it. > > This is another instance of the "how much detail to include" we > discussed above. > I'm fine with the brevity here ... it's not untrue, and if you need details, they're not hard to find. (Not sure it's worth repeating a more elaborate blurb in twenty places.) >>>> name = expr['enum'] >>>> members = expr['data'] >>>> prefix = expr.get('prefix') >>>> @@ -273,6 +427,14 @@ def check_enum(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> >>>> >>>> def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> + """ >>>> + Validate this expression as a ``struct`` definition. >>>> + >>>> + :param expr: The expression to validate. >>>> + :param info: QAPI source file information. >>>> + >>>> + :return: None, ``expr`` is normalized in-place as needed. >>>> + """ >>>> name = cast(str, expr['struct']) # Asserted in check_exprs >>>> members = expr['data'] >>>> >>>> @@ -281,6 +443,14 @@ def check_struct(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> >>>> >>>> def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> + """ >>>> + Validate this expression as a ``union`` definition. >>>> + >>>> + :param expr: The expression to validate. >>>> + :param info: QAPI source file information. >>>> + >>>> + :return: None, ``expr`` is normalized in-place as needed. >>>> + """ >>>> name = cast(str, expr['union']) # Asserted in check_exprs >>>> base = expr.get('base') >>>> discriminator = expr.get('discriminator') >>>> @@ -309,6 +479,14 @@ def check_union(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> >>>> >>>> def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> + """ >>>> + Validate this expression as an ``alternate`` definition. >>>> + >>>> + :param expr: The expression to validate. >>>> + :param info: QAPI source file information. >>>> + >>>> + :return: None, ``expr`` is normalized in-place as needed. >>>> + """ >>>> members = expr['data'] >>>> >>>> if not members: >>>> @@ -326,6 +504,14 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> >>>> >>>> def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> + """ >>>> + Validate this expression as a ``command`` definition. >>>> + >>>> + :param expr: The expression to validate. >>>> + :param info: QAPI source file information. >>>> + >>>> + :return: None, ``expr`` is normalized in-place as needed. >>>> + """ >>>> args = expr.get('data') >>>> rets = expr.get('returns') >>>> boxed = expr.get('boxed', False) >>>> @@ -337,6 +523,14 @@ def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> >>>> >>>> def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> + """ >>>> + Normalize and validate this expression as an ``event`` definition. >>>> + >>>> + :param expr: The expression to validate. >>>> + :param info: QAPI source file information. >>>> + >>>> + :return: None, ``expr`` is normalized in-place as needed. >>>> + """ >>>> args = expr.get('data') >>>> boxed = expr.get('boxed', False) >>>> >>>> @@ -346,6 +540,15 @@ def check_event(expr: _JSONObject, info: QAPISourceInfo) -> None: >>>> >>>> >>>> def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: >>>> + """ >>>> + Validate and normalize a list of parsed QAPI schema expressions. >>>> + >>>> + This function accepts a list of expressions + metadta as returned by >>> >>> Typo: metadata. >>> >> >> I've invented a new kind of data, actually. >> >> (Fixed.) >> >>>> + the parser. It destructively normalizes the expressions in-place. >>>> + >>>> + :param exprs: The list of expressions to normalize/validate. >>>> + :return: The same list of expressions (now modified). >>>> + """ >>>> for expr_elem in exprs: >>>> # Expression >>>> assert isinstance(expr_elem['expr'], dict) >> >> Made a bunch of changes, but held off on trying to "finish" it; I want >> to make a checklist for myself to review with your counter-feedback and >> methodically revise it all in one shot. > > Makes sense. > >> Thanks! > > Glad the effort is of some use! > I've made a re-spin. Let's try something new, if you don't mind: I've pushed a "almost v5" copy onto my gitlab, where edits made against this patch are in their own commit so that all of the pending edits I've made are easily visible. Here's the "merge request", which I made against my own fork of master: https://gitlab.com/jsnow/qemu/-/merge_requests/1/diffs (It's marked "WIP", so there's no risk of me accidentally merging it -- and if I did, it would be to my own "master" branch, so no worries about us goofing this up.) If you click "Commits (21)" at the top, underneath "WIP: python-qapi-cleanup-pt3", you can see the list of commits in the re-spin. (Four of these commits are the DO-NOT-MERGE ones I carry around as a testing pre-requisite.) From here, you can see the "[RFC] docstring diff" patch which shows all the edits I've made so far based on your feedback and my tinkering. https://gitlab.com/jsnow/qemu/-/merge_requests/1/diffs?commit_id=3f0e9fb71304edb381ce3b9bf0ff08624fb277bc I invite you to leave feedback here on this view (and anywhere else in the series that still needs adjusting, if you are so willing to humor me) by highlighting the line and clicking the comment box icon on the left. If you left-click and drag the comment box, you can target a range of lines. (You can even propose a diff directly using this method, which allows me to just accept your proposal directly.) If you leave any comments here, I can resolve each individual nugget of feedback by clicking "Resolve Thread" in my view, which will help me keep track of which items I believe I have addressed and which items I have not. This will help me make sure I don't miss any of your feedback, and it helps me keep track of what edits I've made for the next changelog. Willing to try it out? Once we're both happy with it, I will send it back to the list for final assessment using our traditional process. Anyone else who wants to come comment on the gitlab draft is of course more than welcome to. --js
John Snow <jsnow@redhat.com> writes: [...] > I've made a re-spin. Let's try something new, if you don't mind: > > I've pushed a "almost v5" copy onto my gitlab, where edits made against > this patch are in their own commit so that all of the pending edits I've > made are easily visible. > > Here's the "merge request", which I made against my own fork of master: > https://gitlab.com/jsnow/qemu/-/merge_requests/1/diffs > > (It's marked "WIP", so there's no risk of me accidentally merging it -- > and if I did, it would be to my own "master" branch, so no worries about > us goofing this up.) > > If you click "Commits (21)" at the top, underneath "WIP: > python-qapi-cleanup-pt3", you can see the list of commits in the re-spin. > > (Four of these commits are the DO-NOT-MERGE ones I carry around as a > testing pre-requisite.) > > From here, you can see the "[RFC] docstring diff" patch which shows all > the edits I've made so far based on your feedback and my tinkering. > > https://gitlab.com/jsnow/qemu/-/merge_requests/1/diffs?commit_id=3f0e9fb71304edb381ce3b9bf0ff08624fb277bc > > I invite you to leave feedback here on this view (and anywhere else in > the series that still needs adjusting, if you are so willing to humor > me) by highlighting the line and clicking the comment box icon on the > left. If you left-click and drag the comment box, you can target a range > of lines. > > (You can even propose a diff directly using this method, which allows me > to just accept your proposal directly.) > > If you leave any comments here, I can resolve each individual nugget of > feedback by clicking "Resolve Thread" in my view, which will help me > keep track of which items I believe I have addressed and which items I > have not. This will help me make sure I don't miss any of your feedback, > and it helps me keep track of what edits I've made for the next changelog. > > Willing to try it out? > > Once we're both happy with it, I will send it back to the list for final > assessment using our traditional process. Anyone else who wants to come > comment on the gitlab draft is of course more than welcome to. I have only a few minor remarks, and I'm too lazy to create a gitlab account just for them. * Commit 3f0e9fb713 qapi/expr: [RFC] docstring diff - You mixed up check_name_lower() and check_name_camel() - Nitpick: check_defn_name_str() has inconsistent function name markup. - I'd like to suggest a tweak of check_defn_name_str() :param meta: That's all. Converged quickly. Nice! Incremental diff appended. * Old "[PATCH v4 17/19] qapi/expr.py: Use tuples instead of lists for static data" is gone. I think this leaves commit 913e3fd6f8's "Later patches will make use of that" dangling. Let's not drop old PATCH 17. Put it right after 913e3fd6f8 if that's trivial. If not, put it wherever it creates the least work for you. diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index f2bb92ab79..5c9060cb1b 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -124,7 +124,7 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, permit_upper: bool = False, permit_underscore: bool = False) -> None: """ - Ensure that ``name`` is a valid user defined type name. + Ensure that ``name`` is a valid command or member name. This means it must be a valid QAPI name as checked by `check_name_str()`, but where the stem prohibits uppercase @@ -147,7 +147,7 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None: """ - Ensure that ``name`` is a valid command or member name. + Ensure that ``name`` is a valid user-defined type name. This means it must be a valid QAPI name as checked by `check_name_str()`, but where the stem must be in CamelCase. @@ -168,14 +168,14 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None: Ensure that ``name`` is a valid definition name. Based on the value of ``meta``, this means that: - - 'event' names adhere to `check_name_upper`. - - 'command' names adhere to `check_name_lower`. + - 'event' names adhere to `check_name_upper()`. + - 'command' names adhere to `check_name_lower()`. - Else, meta is a type, and must pass `check_name_camel()`. These names must not end with ``Kind`` nor ``List``. :param name: Name to check. :param info: QAPI schema source file information. - :param meta: Type name of the QAPI expression. + :param meta: Meta-type name of the QAPI expression. :raise QAPISemError: When ``name`` fails validation. """
On 4/21/21 9:58 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > > [...] >> I've made a re-spin. Let's try something new, if you don't mind: >> >> I've pushed a "almost v5" copy onto my gitlab, where edits made against >> this patch are in their own commit so that all of the pending edits I've >> made are easily visible. >> >> Here's the "merge request", which I made against my own fork of master: >> https://gitlab.com/jsnow/qemu/-/merge_requests/1/diffs >> >> (It's marked "WIP", so there's no risk of me accidentally merging it -- >> and if I did, it would be to my own "master" branch, so no worries about >> us goofing this up.) >> >> If you click "Commits (21)" at the top, underneath "WIP: >> python-qapi-cleanup-pt3", you can see the list of commits in the re-spin. >> >> (Four of these commits are the DO-NOT-MERGE ones I carry around as a >> testing pre-requisite.) >> >> From here, you can see the "[RFC] docstring diff" patch which shows all >> the edits I've made so far based on your feedback and my tinkering. >> >> https://gitlab.com/jsnow/qemu/-/merge_requests/1/diffs?commit_id=3f0e9fb71304edb381ce3b9bf0ff08624fb277bc >> >> I invite you to leave feedback here on this view (and anywhere else in >> the series that still needs adjusting, if you are so willing to humor >> me) by highlighting the line and clicking the comment box icon on the >> left. If you left-click and drag the comment box, you can target a range >> of lines. >> >> (You can even propose a diff directly using this method, which allows me >> to just accept your proposal directly.) >> >> If you leave any comments here, I can resolve each individual nugget of >> feedback by clicking "Resolve Thread" in my view, which will help me >> keep track of which items I believe I have addressed and which items I >> have not. This will help me make sure I don't miss any of your feedback, >> and it helps me keep track of what edits I've made for the next changelog. >> >> Willing to try it out? >> >> Once we're both happy with it, I will send it back to the list for final >> assessment using our traditional process. Anyone else who wants to come >> comment on the gitlab draft is of course more than welcome to. > > I have only a few minor remarks, and I'm too lazy to create a gitlab > account just for them. > (You'll need one eventually, I think. There was talk of requiring maintainers all to have one in order to run CI by submitting a MR on the main repo as an alternative PR workflow up to Peter Maydell to reduce CI hours. Humor me and make one? I really would like to try it out. Maybe for part 5? I want to see if it helps me be more organized when making a large number of edits, and I think it might help me implement more of your suggestions. At least, that's how I'm selling it!) > * Commit 3f0e9fb713 qapi/expr: [RFC] docstring diff > > - You mixed up check_name_lower() and check_name_camel() > @_@ oops. Too many nearly identical code regions. Thanks. > - Nitpick: check_defn_name_str() has inconsistent function name > markup. > Good spot. It doesn't matter to sphinx, but you expressed a preference for seeing the empty parens to help intuit the type of symbol being referenced when reading the plaintext and I agree. > - I'd like to suggest a tweak of check_defn_name_str() :param meta: > Sounds good, thanks! Anything that makes "type" less ambiguous is graciously welcome. > That's all. Converged quickly. Nice! Incremental diff appended. > > * Old "[PATCH v4 17/19] qapi/expr.py: Use tuples instead of lists for > static data" is gone. I think this leaves commit 913e3fd6f8's "Later > patches will make use of that" dangling. Let's not drop old PATCH 17. > Put it right after 913e3fd6f8 if that's trivial. If not, put it > wherever it creates the least work for you. > OK, I can un-plunk it. > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index f2bb92ab79..5c9060cb1b 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -124,7 +124,7 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, > permit_upper: bool = False, > permit_underscore: bool = False) -> None: > """ > - Ensure that ``name`` is a valid user defined type name. > + Ensure that ``name`` is a valid command or member name. > > This means it must be a valid QAPI name as checked by > `check_name_str()`, but where the stem prohibits uppercase > @@ -147,7 +147,7 @@ def check_name_lower(name: str, info: QAPISourceInfo, source: str, > > def check_name_camel(name: str, info: QAPISourceInfo, source: str) -> None: > """ > - Ensure that ``name`` is a valid command or member name. > + Ensure that ``name`` is a valid user-defined type name. > > This means it must be a valid QAPI name as checked by > `check_name_str()`, but where the stem must be in CamelCase. > @@ -168,14 +168,14 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None: > Ensure that ``name`` is a valid definition name. > > Based on the value of ``meta``, this means that: > - - 'event' names adhere to `check_name_upper`. > - - 'command' names adhere to `check_name_lower`. > + - 'event' names adhere to `check_name_upper()`. > + - 'command' names adhere to `check_name_lower()`. > - Else, meta is a type, and must pass `check_name_camel()`. > These names must not end with ``Kind`` nor ``List``. > > :param name: Name to check. > :param info: QAPI schema source file information. > - :param meta: Type name of the QAPI expression. > + :param meta: Meta-type name of the QAPI expression. > > :raise QAPISemError: When ``name`` fails validation. > """ > Thanks! The list of fixes this time was indeed short enough to keep track of the old way :) Re-pushing without the "pt0" pre-req to my gitlab for CI reasons, and sending a proper V5 to the list. --js For giggles, I updated that "merge request" also so I could see how it tracks patchset diffs and changelogs and stuff. Or, rather, it's the case that by force-pushing to the same branch to run CI, it automatically creates a new "version" of the MR, but I did have to update the "cover letter" myself. You can see what that looks like at https://gitlab.com/jsnow/qemu/-/merge_requests/1