| Message ID | 20210325060356.4040114-17-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt3 | expand |
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)
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/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 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
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)
Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 213 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 208 insertions(+), 5 deletions(-)