| Message ID | 20200930043150.1454766-1-jsnow@redhat.com |
|---|---|
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
John Snow <jsnow@redhat.com> writes: > I did not say "sphinx beautiful", just "sphinx compatible". They will > not throw errors when parsed and interpreted as ReST. "Bang on the keyboard until Sphinx doesn't throw errors anymore" might be good enough for a certain kind of mathematician, but a constructive solution needs a bit more direction. Is there a specification to follow? Other useful resources? > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/gen.py | 6 ++++-- > scripts/qapi/parser.py | 9 +++++---- > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py > index ca66c82b5b8..fc19b2aeb9b 100644 > --- a/scripts/qapi/gen.py > +++ b/scripts/qapi/gen.py > @@ -154,9 +154,11 @@ def _bottom(self): > > @contextmanager > def ifcontext(ifcond, *args): > - """A 'with' statement context manager to wrap with start_if()/end_if() > + """ > + A 'with' statement context manager that wraps with `start_if` and `end_if`. Sadly, the fact that start_if() and end_if() are functions isn't immediately obvious anymore. I've seen :func:`start_if` elsewhere. Is this something we should or want to use? > > - *args: any number of QAPIGenCCode > + :param ifcond: List of conditionals > + :param args: any number of `QAPIGenCCode`. > > Example:: > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 9d1a3e2eea9..02983979965 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -381,10 +381,11 @@ def append(self, line): > > The way that the line is dealt with depends on which part of > the documentation we're parsing right now: > - * The body section: ._append_line is ._append_body_line > - * An argument section: ._append_line is ._append_args_line > - * A features section: ._append_line is ._append_features_line > - * An additional section: ._append_line is ._append_various_line > + > + * The body section: ._append_line is ._append_body_line > + * An argument section: ._append_line is ._append_args_line > + * A features section: ._append_line is ._append_features_line > + * An additional section: ._append_line is ._append_various_line > """ > line = line[1:] > if not line: I understand why you insert a blank line (reST wants blank lines around lists), I don't understand why you indent. Can you explain?
On Wed, Sep 30, 2020 at 12:31:34AM -0400, John Snow wrote: > The edge case is that if the name is '', this expression returns a > string instead of a bool, which violates our declared type. > > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Cleber Rosa <crosa@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
On Wed, Sep 30, 2020 at 12:31:42AM -0400, John Snow wrote: > _make_tree might receive a dict or some other type. Adding features > information should arguably be performed by the caller at such a time > when we know the type of the object and don't have to re-interrogate it. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/introspect.py | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index f7de3953391..5cbdc7414bd 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -52,16 +52,12 @@ > > > def _make_tree(obj: Union[_DObject, str], ifcond: List[str], > - features: List[QAPISchemaFeature], > extra: Optional[Extra] = None > ) -> Union[TreeNode, AnnotatedNode]: > if extra is None: > extra = {} > if ifcond: > extra['if'] = ifcond > - if features: > - assert isinstance(obj, dict) > - obj['features'] = [(f.name, {'if': f.ifcond}) for f in features] Now the reason for moving this code outside _make_tree() is more obvious due to the type annotations. Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > if extra: > return (obj, extra) > return obj > @@ -197,6 +193,11 @@ def _use_type(self, typ: QAPISchemaType) -> str: > return '[' + self._use_type(typ.element_type) + ']' > return self._name(typ.name) > > + @classmethod > + def _gen_features(cls, > + features: List[QAPISchemaFeature]) -> List[TreeNode]: > + return [_make_tree(f.name, f.ifcond) for f in features] > + > def _gen_tree(self, name: str, mtype: str, obj: _DObject, > ifcond: List[str], > features: Optional[List[QAPISchemaFeature]]) -> None: > @@ -209,7 +210,9 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, > name = self._name(name) > obj['name'] = name > obj['meta-type'] = mtype > - self._trees.append(_make_tree(obj, ifcond, features, extra)) > + if features: > + obj['features'] = self._gen_features(features) > + self._trees.append(_make_tree(obj, ifcond, extra)) > > def _gen_member(self, > member: QAPISchemaObjectTypeMember) -> TreeNode: > @@ -219,7 +222,9 @@ def _gen_member(self, > } > if member.optional: > obj['default'] = None > - return _make_tree(obj, member.ifcond, member.features) > + if member.features: > + obj['features'] = self._gen_features(member.features) > + return _make_tree(obj, member.ifcond) > > def _gen_variants(self, tag_name: str, > variants: List[QAPISchemaVariant]) -> _DObject: > @@ -231,7 +236,7 @@ def _gen_variant(self, variant: QAPISchemaVariant) -> TreeNode: > 'case': variant.name, > 'type': self._use_type(variant.type) > } > - return _make_tree(obj, variant.ifcond, None) > + return _make_tree(obj, variant.ifcond) > > def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo], > json_type: str) -> None: > -- > 2.26.2 >
On 9/30/20 4:47 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> I did not say "sphinx beautiful", just "sphinx compatible". They will >> not throw errors when parsed and interpreted as ReST. > > "Bang on the keyboard until Sphinx doesn't throw errors anymore" might > be good enough for a certain kind of mathematician, but a constructive > solution needs a bit more direction. Is there a specification to > follow? Other useful resources? > I don't know if you are asking this question rhetorically, or in good faith. Let me preface this by saying: This series, and these 119 patches, are not about finding a style guide for our docstring utilization or about proposing one. It is also not about rigorously adding such documentation or about finding ways to meaningfully publish it with e.g. Sphinx, or the styling of such pages. Why bother to add docstrings at all, then? Because I needed them for my own sake when learning this code and I felt it would be a waste to just delete them, and I am of sound mind and able body and believe that some documentation was better than none. They are useful even just as plaintext. Having said that, let's explore the actual style I tend to use. I mentioned before in response to a review comment that there isn't really any standard for docstrings. There are a few competing "styles", but none of which are able to be programmatically checked and validated. The primary guide for docstrings is PEP 257, of which I follow some but not all of the recommendations. https://www.python.org/dev/peps/pep-0257/ In general, - Always use triple-double quotes (unenforced) - Modules, classes, and functions should have docstrings (pylint) - No empty lines before or after the docstring (unenforced) - Multi-line docstrings should take the form (unenforced): """ single-line summary of function. Additional prose if needed describing the function. :param x: Input such that blah blah. :raises y: When input ``x`` is unsuitable because blah blah. :returns: A value that blah blah. """ PEP257 suggests a form where the single-line summary appears on the same line as the opening triple quotes. I don't like this, and prefer symmetry. PEP257 *also* suggests that writing it my way is equivalent to their way, because any docstring processor should trim the first line. I take this as tacit admission that my way is acceptable and has merit. What about the syntax or markup inside the docstring itself? there is *absolutely no standard*, but Sphinx autodoc recognizes a few field lists as significant in its parsing, so I prefer using them: :param x: Denotes the parameter X. Do not use type information in the string, we rely on mypy for that now. :raises y: explains a case in which an Exception type y may be raised either directly by this code or anticipated to be allowed to be raised by a helper call. (There's no standard here that I am aware of. I use my judgment. Always document direct raise calls, but use your judgment for sub-calls.) :returns: explains the semantics of the return value. That said, literally any sphinx/ReST markup is OK as long as it passes make sphinxdocs. Some sphinx markup is prohibited, like adding new full-throated sections. You can use arbitrary field lists, definition lists, pre-formatted text, examples, code blocks, whatever. In general, you are not going to find the kind of semantic validation you want to ensure that the parameter names are correct, or that you spelled :param: right, or that you didn't miss a parameter or an exception. None of that tooling exists for Python. Thus, it's all rather subjective. No right answers, no validation tools. Just whatever seems reasonable to a human eye until such time we actually decide to pursue publishing the API docs in the development manual, if indeed we ever do so at all. That series sounds like a great opportunity to hash this all out. That is when I would like to remove --missing-docstring, too. There will absolutely be a "docstring series" in the future, but I am insisting stubbornly it happen after strict typing. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/gen.py | 6 ++++-- >> scripts/qapi/parser.py | 9 +++++---- >> 2 files changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >> index ca66c82b5b8..fc19b2aeb9b 100644 >> --- a/scripts/qapi/gen.py >> +++ b/scripts/qapi/gen.py >> @@ -154,9 +154,11 @@ def _bottom(self): >> >> @contextmanager >> def ifcontext(ifcond, *args): >> - """A 'with' statement context manager to wrap with start_if()/end_if() >> + """ >> + A 'with' statement context manager that wraps with `start_if` and `end_if`. > > Sadly, the fact that start_if() and end_if() are functions isn't > immediately obvious anymore. > > I've seen :func:`start_if` elsewhere. Is this something we should or > want to use? > We *could*. `start_if` relies on the default role, which I have provisionally set to "any" here, so this is shorthand for :any:`start_if`. The :any: role means: "cross-reference any type of thing." If there is not exactly one thing that matches, it results in an error during the documentation build. I like this, because it's nice short-hand syntax that I think communicates effectively to the reader that this is a symbol of some kind without needing a premium of ReST-ese. CONSTANTS are capitalized, Classes are title cased, and functions are lower_case. `lower_case` references can be assumed to be functions, but I will admit that this is not enforced or necessarily true as we add more cross reference types in the future. (I am trying to add QMP cross-reference syntax!) I still prefer `start_if` to :func:`start_if` simply because it's less markup and is easier to read in plaintext contexts. You're right, it doesn't look like a function anymore. I'm not sure if another annotations would work -- `start_if`() or `start_if()`. Both seem kind of clunky to me, to be honest. Personal feeling is "not really worth the hassle." >> >> - *args: any number of QAPIGenCCode >> + :param ifcond: List of conditionals >> + :param args: any number of `QAPIGenCCode`. >> >> Example:: >> >> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >> index 9d1a3e2eea9..02983979965 100644 >> --- a/scripts/qapi/parser.py >> +++ b/scripts/qapi/parser.py >> @@ -381,10 +381,11 @@ def append(self, line): >> >> The way that the line is dealt with depends on which part of >> the documentation we're parsing right now: >> - * The body section: ._append_line is ._append_body_line >> - * An argument section: ._append_line is ._append_args_line >> - * A features section: ._append_line is ._append_features_line >> - * An additional section: ._append_line is ._append_various_line >> + >> + * The body section: ._append_line is ._append_body_line >> + * An argument section: ._append_line is ._append_args_line >> + * A features section: ._append_line is ._append_features_line >> + * An additional section: ._append_line is ._append_various_line >> """ >> line = line[1:] >> if not line: > > I understand why you insert a blank line (reST wants blank lines around > lists), I don't understand why you indent. Can you explain? I was mistaken about it needing the indent! --js
On 9/30/20 4:47 AM, Markus Armbruster wrote: > Sadly, the fact that start_if() and end_if() are functions isn't > immediately obvious anymore. > > I've seen :func:`start_if` elsewhere. Is this something we should or > want to use? Looks like `start_if()` works just fine too. If you are hard-set in wanting to see those parentheses I can use this style, but admit I am not likely to use them myself in newer docstrings, and there's no way to enforce their presence OR absence that I am aware of. I'll bake that change in for now until I see another reply. --js
On Wed, Sep 30, 2020 at 12:31:41AM -0400, John Snow wrote: > From: Eduardo Habkost <ehabkost@redhat.com> > > The typing of _make_tree and friends is a bit involved, but it can be > done with some stubbed out types and a bit of elbow grease. The > forthcoming patches attempt to make some simplifications, but having the > type hints in advance can aid in review. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
On 9/30/20 2:39 PM, Eduardo Habkost wrote: > On Wed, Sep 30, 2020 at 12:31:45AM -0400, John Snow wrote: >> This replaces _make_tree with Node.__init__(), effectively. By creating >> it as a generic container, we can more accurately describe the exact >> nature of this particular Node. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/introspect.py | 77 +++++++++++++++++++------------------- >> 1 file changed, 38 insertions(+), 39 deletions(-) >> >> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >> index 43b6ba5df1f..86286e755ca 100644 >> --- a/scripts/qapi/introspect.py >> +++ b/scripts/qapi/introspect.py >> @@ -12,11 +12,12 @@ >> >> from typing import ( >> Dict, >> + Generic, >> + Iterable, >> List, >> Optional, >> Sequence, >> - Tuple, >> - Union, >> + TypeVar, >> ) >> >> from .common import ( >> @@ -43,42 +44,42 @@ >> >> >> # The correct type for TreeNode is actually: >> -# Union[AnnotatedNode, List[TreeNode], Dict[str, TreeNode], str, bool] >> +# Union[Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool] >> # but mypy does not support recursive types yet. >> TreeNode = object >> +_NodeType = TypeVar('_NodeType', bound=TreeNode) >> _DObject = Dict[str, object] >> -Extra = Dict[str, object] >> -AnnotatedNode = Tuple[TreeNode, Extra] > > Do you have plans to make Node replace TreeNode completely? > > I'd understand this patch as a means to reach that goal, but I'm > not sure the intermediate state of having both Node and TreeNode > types (that can be confused with each other) is desirable. > The problem is that _tree_to_qlit still accepts a broad array of types. The "TreeNode" comment above explains that those types are: Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool Three are containers, two are leaf values. of the containers, the Node container is special in that it houses explicitly one of the four other types (but never itself.) Even if I somehow always enforced Node[T] heading into _tree_to_qlit, I would still need to describe what 'T' is, which is another recursive type that I cannot exactly describe with mypy's current descriptive power: INNER_TYPE = List[Node[INNER_TYPE]], Dict[str, Node[INNER_TYPE]], str, bool And that's not really a huge win, or indeed any different to the existing TreeNode being an "object". So ... no, I felt like I was going to stop here, where we have fundamentally: 1. Undecorated nodes (list, dict, str, bool) ("TreeNode") 2. Decorated nodes (Node[T]) ("Node") which leads to the question: Why bother swapping Tuple for Node at that point? My answer is simply that having a strong type name allows us to distinguish this from garden-variety Tuples that might sneak in for other reasons in other data types. Maybe we want a different nomenclature though, like Node vs AnnotatedNode? --js (Also: 'TreeNode' is just an alias for object, it doesn't mean anything grammatically. I could just as soon erase it entirely if you felt it provided no value. It doesn't enforce that it only takes objects we declared were 'TreeNode' types, for instance. It's just a preprocessor name, basically.) >> >> >> -def _make_tree(obj: Union[_DObject, str], ifcond: List[str], >> - comment: Optional[str] = None) -> AnnotatedNode: >> - extra: Extra = { >> - 'if': ifcond, >> - 'comment': comment, >> - } >> - return (obj, extra) >> +class Node(Generic[_NodeType]): >> + """ >> + Node generally contains a SchemaInfo-like type (as a dict), >> + But it also used to wrap comments/ifconds around leaf value types. >> + """ >> + # Remove after 3.7 adds @dataclass: >> + # pylint: disable=too-few-public-methods >> + def __init__(self, data: _NodeType, ifcond: Iterable[str], >> + comment: Optional[str] = None): >> + self.data = data >> + self.comment: Optional[str] = comment >> + self.ifcond: Sequence[str] = tuple(ifcond) >> >> >> -def _tree_to_qlit(obj: TreeNode, >> - level: int = 0, >> +def _tree_to_qlit(obj: TreeNode, level: int = 0, >> suppress_first_indent: bool = False) -> str: >> >> def indent(level: int) -> str: >> return level * 4 * ' ' >> >> - if isinstance(obj, tuple): >> - ifobj, extra = obj >> - ifcond = extra.get('if') >> - comment = extra.get('comment') >> + if isinstance(obj, Node): >> ret = '' >> - if comment: >> - ret += indent(level) + '/* %s */\n' % comment >> - if ifcond: >> - ret += gen_if(ifcond) >> - ret += _tree_to_qlit(ifobj, level) >> - if ifcond: >> - ret += '\n' + gen_endif(ifcond) >> + if obj.comment: >> + ret += indent(level) + '/* %s */\n' % obj.comment >> + if obj.ifcond: >> + ret += gen_if(obj.ifcond) >> + ret += _tree_to_qlit(obj.data, level) >> + if obj.ifcond: >> + ret += '\n' + gen_endif(obj.ifcond) >> return ret >> >> ret = '' >> @@ -125,7 +126,7 @@ def __init__(self, prefix: str, unmask: bool): >> ' * QAPI/QMP schema introspection', __doc__) >> self._unmask = unmask >> self._schema: Optional[QAPISchema] = None >> - self._trees: List[AnnotatedNode] = [] >> + self._trees: List[Node[_DObject]] = [] >> self._used_types: List[QAPISchemaType] = [] >> self._name_map: Dict[str, str] = {} >> self._genc.add(mcgen(''' >> @@ -192,9 +193,8 @@ def _use_type(self, typ: QAPISchemaType) -> str: >> >> @classmethod >> def _gen_features(cls, >> - features: List[QAPISchemaFeature] >> - ) -> List[AnnotatedNode]: >> - return [_make_tree(f.name, f.ifcond) for f in features] >> + features: List[QAPISchemaFeature]) -> List[Node[str]]: >> + return [Node(f.name, f.ifcond) for f in features] >> >> def _gen_tree(self, name: str, mtype: str, obj: _DObject, >> ifcond: List[str], >> @@ -210,10 +210,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, >> obj['meta-type'] = mtype >> if features: >> obj['features'] = self._gen_features(features) >> - self._trees.append(_make_tree(obj, ifcond, comment)) >> + self._trees.append(Node(obj, ifcond, comment)) >> >> def _gen_member(self, >> - member: QAPISchemaObjectTypeMember) -> AnnotatedNode: >> + member: QAPISchemaObjectTypeMember) -> Node[_DObject]: >> obj: _DObject = { >> 'name': member.name, >> 'type': self._use_type(member.type) >> @@ -222,19 +222,19 @@ def _gen_member(self, >> obj['default'] = None >> if member.features: >> obj['features'] = self._gen_features(member.features) >> - return _make_tree(obj, member.ifcond) >> + return Node(obj, member.ifcond) >> >> def _gen_variants(self, tag_name: str, >> variants: List[QAPISchemaVariant]) -> _DObject: >> return {'tag': tag_name, >> 'variants': [self._gen_variant(v) for v in variants]} >> >> - def _gen_variant(self, variant: QAPISchemaVariant) -> AnnotatedNode: >> + def _gen_variant(self, variant: QAPISchemaVariant) -> Node[_DObject]: >> obj: _DObject = { >> 'case': variant.name, >> 'type': self._use_type(variant.type) >> } >> - return _make_tree(obj, variant.ifcond) >> + return Node(obj, variant.ifcond) >> >> def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo], >> json_type: str) -> None: >> @@ -245,8 +245,7 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo, >> members: List[QAPISchemaEnumMember], >> prefix: Optional[str]) -> None: >> self._gen_tree(name, 'enum', >> - {'values': [_make_tree(m.name, m.ifcond, None) >> - for m in members]}, >> + {'values': [Node(m.name, m.ifcond) for m in members]}, >> ifcond, features) >> >> def visit_array_type(self, name: str, info: Optional[QAPISourceInfo], >> @@ -274,9 +273,9 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo, >> variants: QAPISchemaVariants) -> None: >> self._gen_tree(name, 'alternate', >> {'members': [ >> - _make_tree({'type': self._use_type(m.type)}, >> - m.ifcond, None) >> - for m in variants.variants]}, >> + Node({'type': self._use_type(m.type)}, m.ifcond) >> + for m in variants.variants >> + ]}, >> ifcond, features) >> >> def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str], >> -- >> 2.26.2 >> >
On Wed, Sep 30, 2020 at 02:58:04PM -0400, John Snow wrote: > On 9/30/20 2:39 PM, Eduardo Habkost wrote: > > On Wed, Sep 30, 2020 at 12:31:45AM -0400, John Snow wrote: > > > This replaces _make_tree with Node.__init__(), effectively. By creating > > > it as a generic container, we can more accurately describe the exact > > > nature of this particular Node. > > > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > > --- > > > scripts/qapi/introspect.py | 77 +++++++++++++++++++------------------- > > > 1 file changed, 38 insertions(+), 39 deletions(-) > > > > > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > > > index 43b6ba5df1f..86286e755ca 100644 > > > --- a/scripts/qapi/introspect.py > > > +++ b/scripts/qapi/introspect.py > > > @@ -12,11 +12,12 @@ > > > from typing import ( > > > Dict, > > > + Generic, > > > + Iterable, > > > List, > > > Optional, > > > Sequence, > > > - Tuple, > > > - Union, > > > + TypeVar, > > > ) > > > from .common import ( > > > @@ -43,42 +44,42 @@ > > > # The correct type for TreeNode is actually: > > > -# Union[AnnotatedNode, List[TreeNode], Dict[str, TreeNode], str, bool] > > > +# Union[Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool] > > > # but mypy does not support recursive types yet. > > > TreeNode = object > > > +_NodeType = TypeVar('_NodeType', bound=TreeNode) > > > _DObject = Dict[str, object] > > > -Extra = Dict[str, object] > > > -AnnotatedNode = Tuple[TreeNode, Extra] > > > > Do you have plans to make Node replace TreeNode completely? > > > > I'd understand this patch as a means to reach that goal, but I'm > > not sure the intermediate state of having both Node and TreeNode > > types (that can be confused with each other) is desirable. > > > > The problem is that _tree_to_qlit still accepts a broad array of types. The > "TreeNode" comment above explains that those types are: > > Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool > > Three are containers, two are leaf values. > of the containers, the Node container is special in that it houses > explicitly one of the four other types (but never itself.) > > Even if I somehow always enforced Node[T] heading into _tree_to_qlit, I > would still need to describe what 'T' is, which is another recursive type > that I cannot exactly describe with mypy's current descriptive power: > > INNER_TYPE = List[Node[INNER_TYPE]], Dict[str, Node[INNER_TYPE]], str, bool > > And that's not really a huge win, or indeed any different to the existing > TreeNode being an "object". > > So ... no, I felt like I was going to stop here, where we have > fundamentally: > > 1. Undecorated nodes (list, dict, str, bool) ("TreeNode") > 2. Decorated nodes (Node[T]) ("Node") > > which leads to the question: Why bother swapping Tuple for Node at that > point? > > My answer is simply that having a strong type name allows us to distinguish > this from garden-variety Tuples that might sneak in for other reasons in > other data types. Would: AnnotatedNode = NewType('AnnotatedNode', Tuple[TreeNode, Extra]) be enough, then? > > Maybe we want a different nomenclature though, like Node vs AnnotatedNode? Yes, I believe having a more explicit name would be better. > > --js > > (Also: 'TreeNode' is just an alias for object, it doesn't mean anything > grammatically. I could just as soon erase it entirely if you felt it > provided no value. It doesn't enforce that it only takes objects we declared > were 'TreeNode' types, for instance. It's just a preprocessor name, > basically.) > > > > -def _make_tree(obj: Union[_DObject, str], ifcond: List[str], > > > - comment: Optional[str] = None) -> AnnotatedNode: > > > - extra: Extra = { > > > - 'if': ifcond, > > > - 'comment': comment, > > > - } > > > - return (obj, extra) > > > +class Node(Generic[_NodeType]): > > > + """ > > > + Node generally contains a SchemaInfo-like type (as a dict), > > > + But it also used to wrap comments/ifconds around leaf value types. > > > + """ > > > + # Remove after 3.7 adds @dataclass: > > > + # pylint: disable=too-few-public-methods > > > + def __init__(self, data: _NodeType, ifcond: Iterable[str], > > > + comment: Optional[str] = None): > > > + self.data = data > > > + self.comment: Optional[str] = comment > > > + self.ifcond: Sequence[str] = tuple(ifcond) > > > -def _tree_to_qlit(obj: TreeNode, > > > - level: int = 0, > > > +def _tree_to_qlit(obj: TreeNode, level: int = 0, > > > suppress_first_indent: bool = False) -> str: > > > def indent(level: int) -> str: > > > return level * 4 * ' ' > > > - if isinstance(obj, tuple): > > > - ifobj, extra = obj > > > - ifcond = extra.get('if') > > > - comment = extra.get('comment') > > > + if isinstance(obj, Node): > > > ret = '' > > > - if comment: > > > - ret += indent(level) + '/* %s */\n' % comment > > > - if ifcond: > > > - ret += gen_if(ifcond) > > > - ret += _tree_to_qlit(ifobj, level) > > > - if ifcond: > > > - ret += '\n' + gen_endif(ifcond) > > > + if obj.comment: > > > + ret += indent(level) + '/* %s */\n' % obj.comment > > > + if obj.ifcond: > > > + ret += gen_if(obj.ifcond) > > > + ret += _tree_to_qlit(obj.data, level) > > > + if obj.ifcond: > > > + ret += '\n' + gen_endif(obj.ifcond) > > > return ret > > > ret = '' > > > @@ -125,7 +126,7 @@ def __init__(self, prefix: str, unmask: bool): > > > ' * QAPI/QMP schema introspection', __doc__) > > > self._unmask = unmask > > > self._schema: Optional[QAPISchema] = None > > > - self._trees: List[AnnotatedNode] = [] > > > + self._trees: List[Node[_DObject]] = [] > > > self._used_types: List[QAPISchemaType] = [] > > > self._name_map: Dict[str, str] = {} > > > self._genc.add(mcgen(''' > > > @@ -192,9 +193,8 @@ def _use_type(self, typ: QAPISchemaType) -> str: > > > @classmethod > > > def _gen_features(cls, > > > - features: List[QAPISchemaFeature] > > > - ) -> List[AnnotatedNode]: > > > - return [_make_tree(f.name, f.ifcond) for f in features] > > > + features: List[QAPISchemaFeature]) -> List[Node[str]]: > > > + return [Node(f.name, f.ifcond) for f in features] > > > def _gen_tree(self, name: str, mtype: str, obj: _DObject, > > > ifcond: List[str], > > > @@ -210,10 +210,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject, > > > obj['meta-type'] = mtype > > > if features: > > > obj['features'] = self._gen_features(features) > > > - self._trees.append(_make_tree(obj, ifcond, comment)) > > > + self._trees.append(Node(obj, ifcond, comment)) > > > def _gen_member(self, > > > - member: QAPISchemaObjectTypeMember) -> AnnotatedNode: > > > + member: QAPISchemaObjectTypeMember) -> Node[_DObject]: > > > obj: _DObject = { > > > 'name': member.name, > > > 'type': self._use_type(member.type) > > > @@ -222,19 +222,19 @@ def _gen_member(self, > > > obj['default'] = None > > > if member.features: > > > obj['features'] = self._gen_features(member.features) > > > - return _make_tree(obj, member.ifcond) > > > + return Node(obj, member.ifcond) > > > def _gen_variants(self, tag_name: str, > > > variants: List[QAPISchemaVariant]) -> _DObject: > > > return {'tag': tag_name, > > > 'variants': [self._gen_variant(v) for v in variants]} > > > - def _gen_variant(self, variant: QAPISchemaVariant) -> AnnotatedNode: > > > + def _gen_variant(self, variant: QAPISchemaVariant) -> Node[_DObject]: > > > obj: _DObject = { > > > 'case': variant.name, > > > 'type': self._use_type(variant.type) > > > } > > > - return _make_tree(obj, variant.ifcond) > > > + return Node(obj, variant.ifcond) > > > def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo], > > > json_type: str) -> None: > > > @@ -245,8 +245,7 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo, > > > members: List[QAPISchemaEnumMember], > > > prefix: Optional[str]) -> None: > > > self._gen_tree(name, 'enum', > > > - {'values': [_make_tree(m.name, m.ifcond, None) > > > - for m in members]}, > > > + {'values': [Node(m.name, m.ifcond) for m in members]}, > > > ifcond, features) > > > def visit_array_type(self, name: str, info: Optional[QAPISourceInfo], > > > @@ -274,9 +273,9 @@ def visit_alternate_type(self, name: str, info: QAPISourceInfo, > > > variants: QAPISchemaVariants) -> None: > > > self._gen_tree(name, 'alternate', > > > {'members': [ > > > - _make_tree({'type': self._use_type(m.type)}, > > > - m.ifcond, None) > > > - for m in variants.variants]}, > > > + Node({'type': self._use_type(m.type)}, m.ifcond) > > > + for m in variants.variants > > > + ]}, > > > ifcond, features) > > > def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str], > > > -- > > > 2.26.2 > > > > > > -- Eduardo
John Snow <jsnow@redhat.com> writes: > On 9/30/20 4:47 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> I did not say "sphinx beautiful", just "sphinx compatible". They will >>> not throw errors when parsed and interpreted as ReST. >> "Bang on the keyboard until Sphinx doesn't throw errors anymore" >> might >> be good enough for a certain kind of mathematician, but a constructive >> solution needs a bit more direction. Is there a specification to >> follow? Other useful resources? >> > > I don't know if you are asking this question rhetorically, or in good faith. I ask to make sure I understand goals and limitations of your doc string work in this series. Also, even a passing to Sphinx becomes more useful when accompanied by a link to relevant documentation. > Let me preface this by saying: This series, and these 119 patches, are > not about finding a style guide for our docstring utilization or about > proposing one. It is also not about rigorously adding such > documentation or about finding ways to meaningfully publish it with > e.g. Sphinx, or the styling of such pages. > > Why bother to add docstrings at all, then? Because I needed them for > my own sake when learning this code and I felt it would be a waste to > just delete them, and I am of sound mind and able body and believe > that some documentation was better than none. They are useful even > just as plaintext. > > Having said that, let's explore the actual style I tend to use. > > I mentioned before in response to a review comment that there isn't > really any standard for docstrings. There are a few competing > "styles", but none of which are able to be programmatically checked > and validated. > > The primary guide for docstrings is PEP 257, of which I follow some > but not all of the recommendations. > > https://www.python.org/dev/peps/pep-0257/ I find PEP 257 frustrating. It leaves me with more questions than answers. > In general, > > - Always use triple-double quotes (unenforced) > - Modules, classes, and functions should have docstrings (pylint) > - No empty lines before or after the docstring (unenforced) > - Multi-line docstrings should take the form (unenforced): > > """ > single-line summary of function. > > Additional prose if needed describing the function. > > :param x: Input such that blah blah. > :raises y: When input ``x`` is unsuitable because blah blah. > :returns: A value that blah blah. This paragraph is already not PEP 257. > """ > > PEP257 suggests a form where the single-line summary appears on the > same line as the opening triple quotes. I don't like this, and prefer > symmetry. PEP257 *also* suggests that writing it my way is equivalent > to their way, because any docstring processor should trim the first > line. I take this as tacit admission that my way is acceptable and has > merit. I prefer the symmetric form myself. > What about the syntax or markup inside the docstring itself? there is > *absolutely no standard*, but Sphinx autodoc recognizes a few field > lists as significant in its parsing, so I prefer using them: Doc link? > :param x: Denotes the parameter X. Do not use type information in the > string, we rely on mypy for that now. > > :raises y: explains a case in which an Exception type y may be raised > either directly by this code or anticipated to be allowed to be raised > by a helper call. (There's no standard here that I am aware of. I use > my judgment. Always document direct raise calls, but use your judgment > for sub-calls.) > > :returns: explains the semantics of the return value. > > That said, literally any sphinx/ReST markup is OK as long as it passes > make sphinxdocs. Some sphinx markup is prohibited, like adding new > full-throated sections. You can use arbitrary field lists, definition > lists, pre-formatted text, examples, code blocks, whatever. > > In general, you are not going to find the kind of semantic validation > you want to ensure that the parameter names are correct, or that you > spelled :param: right, or that you didn't miss a parameter or an > exception. None of that tooling exists for Python. > > Thus, it's all rather subjective. No right answers, no validation > tools. Just whatever seems reasonable to a human eye until such time > we actually decide to pursue publishing the API docs in the > development manual, if indeed we ever do so at all. > > That series sounds like a great opportunity to hash this all out. That > is when I would like to remove --missing-docstring, too. There will > absolutely be a "docstring series" in the future, but I am insisting > stubbornly it happen after strict typing. Okay. Nevertheless, I'd prefer a bit more information in the commit message. Here's my try: qapi: Modify docstrings to be sphinx-compatible I did not say "sphinx beautiful", just "sphinx compatible". They will not throw errors when parsed and interpreted as ReST. Finding a comprehensive style guide for our docstring utilization is left for another day. For now, use field lists recognized by Sphinx autodoc. FIXME link to their documentation > >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> scripts/qapi/gen.py | 6 ++++-- >>> scripts/qapi/parser.py | 9 +++++---- >>> 2 files changed, 9 insertions(+), 6 deletions(-) >>> >>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >>> index ca66c82b5b8..fc19b2aeb9b 100644 >>> --- a/scripts/qapi/gen.py >>> +++ b/scripts/qapi/gen.py >>> @@ -154,9 +154,11 @@ def _bottom(self): >>> @contextmanager >>> def ifcontext(ifcond, *args): >>> - """A 'with' statement context manager to wrap with start_if()/end_if() >>> + """ >>> + A 'with' statement context manager that wraps with `start_if` and `end_if`. >> Sadly, the fact that start_if() and end_if() are functions isn't >> immediately obvious anymore. >> I've seen :func:`start_if` elsewhere. Is this something we should >> or >> want to use? >> > > We *could*. > > `start_if` relies on the default role, which I have provisionally set > to "any" here, so this is shorthand for :any:`start_if`. > > The :any: role means: "cross-reference any type of thing." If there is > not exactly one thing that matches, it results in an error during the > documentation build. > > I like this, because it's nice short-hand syntax that I think > communicates effectively to the reader that this is a symbol of some > kind without needing a premium of ReST-ese. > > CONSTANTS are capitalized, Classes are title cased, and functions are > lower_case. `lower_case` references can be assumed to be functions, `lower_case` could also refer to an attribute, variable, or parameter. > but I will admit that this is not enforced or necessarily true as we > add more cross reference types in the future. > > (I am trying to add QMP cross-reference syntax!) > > I still prefer `start_if` to :func:`start_if` simply because it's less > markup and is easier to read in plaintext contexts. You're right, it > doesn't look like a function anymore. Yes, :func:`start_if` is rather heavy. I asked because I wanted to understand what :func: buys us. Not meant as endorsement. GDK-Doc seems smart enough to recognize start_if(). Sphinx isn't, because it's built around reST syntax. We put our money on the Sphinx horse, so... > I'm not sure if another annotations would work -- `start_if`() or > `start_if()`. Both seem kind of clunky to me, to be honest. Personal > feeling is "not really worth the hassle." You later reported the latter works. I prefer `start_if()` to `start_if`. Matter of taste. > >>> - *args: any number of QAPIGenCCode >>> + :param ifcond: List of conditionals >>> + :param args: any number of `QAPIGenCCode`. >>> Example:: >>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >>> index 9d1a3e2eea9..02983979965 100644 >>> --- a/scripts/qapi/parser.py >>> +++ b/scripts/qapi/parser.py >>> @@ -381,10 +381,11 @@ def append(self, line): >>> The way that the line is dealt with depends on which >>> part of >>> the documentation we're parsing right now: >>> - * The body section: ._append_line is ._append_body_line >>> - * An argument section: ._append_line is ._append_args_line >>> - * A features section: ._append_line is ._append_features_line >>> - * An additional section: ._append_line is ._append_various_line >>> + >>> + * The body section: ._append_line is ._append_body_line >>> + * An argument section: ._append_line is ._append_args_line >>> + * A features section: ._append_line is ._append_features_line >>> + * An additional section: ._append_line is ._append_various_line >>> """ >>> line = line[1:] >>> if not line: >> I understand why you insert a blank line (reST wants blank lines >> around >> lists), I don't understand why you indent. Can you explain? > > I was mistaken about it needing the indent! Easy enough to tidy up :)
John Snow <jsnow@redhat.com> writes: > On 9/30/20 4:47 AM, Markus Armbruster wrote: >> Sadly, the fact that start_if() and end_if() are functions isn't >> immediately obvious anymore. >> I've seen :func:`start_if` elsewhere. Is this something we should >> or >> want to use? > > Looks like `start_if()` works just fine too. If you are hard-set in > wanting to see those parentheses I can use this style, but admit I am > not likely to use them myself in newer docstrings, and there's no way > to enforce their presence OR absence that I am aware of. Well, there's no way to enforce presence or absence of ` either, right? Aside: checking the enclosed text resolves as a cross-reference could provide good-enough enforcement of absence, but enclosing stuff in ` when you shouldn't is probably the lesser issue. > I'll bake that change in for now until I see another reply. Since this is a side show right now, consider the arguments raised so far, use your judgement, and move on to the main show.
On 10/1/20 4:54 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 9/30/20 4:47 AM, Markus Armbruster wrote: >>> Sadly, the fact that start_if() and end_if() are functions isn't >>> immediately obvious anymore. >>> I've seen :func:`start_if` elsewhere. Is this something we should >>> or >>> want to use? >> >> Looks like `start_if()` works just fine too. If you are hard-set in >> wanting to see those parentheses I can use this style, but admit I am >> not likely to use them myself in newer docstrings, and there's no way >> to enforce their presence OR absence that I am aware of. > > Well, there's no way to enforce presence or absence of ` either, right? > Yeah, just a warning that enforcing mechanical consistency along these lines is not something we can do at the moment. (We maybe could by creating a sphinx plugin, but that's a bridge to cross much, much later. Anything is possible with time, engineers, and money, right?) > Aside: checking the enclosed text resolves as a cross-reference could > provide good-enough enforcement of absence, but enclosing stuff in ` > when you shouldn't is probably the lesser issue. > >> I'll bake that change in for now until I see another reply. > > Since this is a side show right now, consider the arguments raised so > far, use your judgement, and move on to the main show. > Keeping emails out of my inbox is presently the nicest thing :) Thanks for looking. --js
On 10/1/20 4:52 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 9/30/20 4:47 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >>> >>>> I did not say "sphinx beautiful", just "sphinx compatible". They will >>>> not throw errors when parsed and interpreted as ReST. >>> "Bang on the keyboard until Sphinx doesn't throw errors anymore" >>> might >>> be good enough for a certain kind of mathematician, but a constructive >>> solution needs a bit more direction. Is there a specification to >>> follow? Other useful resources? >>> >> >> I don't know if you are asking this question rhetorically, or in good faith. > > I ask to make sure I understand goals and limitations of your doc string > work in this series. > > Also, even a passing to Sphinx becomes more useful when accompanied by a > link to relevant documentation. > >> Let me preface this by saying: This series, and these 119 patches, are >> not about finding a style guide for our docstring utilization or about >> proposing one. It is also not about rigorously adding such >> documentation or about finding ways to meaningfully publish it with >> e.g. Sphinx, or the styling of such pages. >> >> Why bother to add docstrings at all, then? Because I needed them for >> my own sake when learning this code and I felt it would be a waste to >> just delete them, and I am of sound mind and able body and believe >> that some documentation was better than none. They are useful even >> just as plaintext. >> >> Having said that, let's explore the actual style I tend to use. >> >> I mentioned before in response to a review comment that there isn't >> really any standard for docstrings. There are a few competing >> "styles", but none of which are able to be programmatically checked >> and validated. >> >> The primary guide for docstrings is PEP 257, of which I follow some >> but not all of the recommendations. >> >> https://www.python.org/dev/peps/pep-0257/ > > I find PEP 257 frustrating. It leaves me with more questions than > answers. > Yeah, sorry. That's what we're dealing with. It's very open-ended. >> In general, >> >> - Always use triple-double quotes (unenforced) >> - Modules, classes, and functions should have docstrings (pylint) >> - No empty lines before or after the docstring (unenforced) >> - Multi-line docstrings should take the form (unenforced): >> >> """ >> single-line summary of function. >> >> Additional prose if needed describing the function. >> >> :param x: Input such that blah blah. >> :raises y: When input ``x`` is unsuitable because blah blah. >> :returns: A value that blah blah. > > This paragraph is already not PEP 257. > Right -- well, it isn't NOT PEP 257 either. It just suggests you have to describe these features, but it doesn't say HOW. >> """ >> >> PEP257 suggests a form where the single-line summary appears on the >> same line as the opening triple quotes. I don't like this, and prefer >> symmetry. PEP257 *also* suggests that writing it my way is equivalent >> to their way, because any docstring processor should trim the first >> line. I take this as tacit admission that my way is acceptable and has >> merit. > > I prefer the symmetric form myself. > >> What about the syntax or markup inside the docstring itself? there is >> *absolutely no standard*, but Sphinx autodoc recognizes a few field >> lists as significant in its parsing, so I prefer using them: > > Doc link? > Hard to search for in my opinion; you want to search for "sphinx python domain", and then click on "field lists" on the sidebar. https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists It has a special understanding for: param/parameter/arg/argument/key/keyword: I prefer "param" here. Possibly key/keyword if we use a **kwargs form with a key that we specially recognize, but I've not tested that yet. I know pycharm understands "param" in a semantic way, and that's been good enough for me. type: Defines the type of a parameter. In my opinion, do not use this. Let native type hints do the lifting. raises/raise/except/exception: I prefer "raises". "raises ErrorType when...." is a good sentence. var/ivar/cvar: Describes a variable, presumably in the body of the function below. I've never used this, I always describe it in prose instead. vartype: Defines a type for a variable; I would again defer to the native type system instead now. returns/return: I prefer "returns" for grammatical reasons again. ("Returns such-and-such when...") rtype: again, type information. Don't use. meta: For directives to sphinx, e.g. :meta private: or :meta public: to toggle the visibility class from its default. I don't use this. None of these are validated or checked in any meaningful way; you can use arbitrarily field lists (and I do in a few places!) to define your own terms and so on. (I would like to improve autodoc in the future to validate your docstrings such that you can enforce :param:, :raises: and :return: and it uses the type hints and introspection information to raise an error when you make an obvious mistake. I am not there yet, but I am using Peter Maydell's work to help inform how I might write such an extension to autodoc. This work is not critical, but it will likely occur upstream, outside of the QEMU context because I believe this is a good thing to do for the ecosystem in general, to allow autodoc to function slightly more like e.g. Doxygen does.) >> :param x: Denotes the parameter X. Do not use type information in the >> string, we rely on mypy for that now. >> >> :raises y: explains a case in which an Exception type y may be raised >> either directly by this code or anticipated to be allowed to be raised >> by a helper call. (There's no standard here that I am aware of. I use >> my judgment. Always document direct raise calls, but use your judgment >> for sub-calls.) >> >> :returns: explains the semantics of the return value. >> >> That said, literally any sphinx/ReST markup is OK as long as it passes >> make sphinxdocs. Some sphinx markup is prohibited, like adding new >> full-throated sections. You can use arbitrary field lists, definition >> lists, pre-formatted text, examples, code blocks, whatever. >> >> In general, you are not going to find the kind of semantic validation >> you want to ensure that the parameter names are correct, or that you >> spelled :param: right, or that you didn't miss a parameter or an >> exception. None of that tooling exists for Python. >> >> Thus, it's all rather subjective. No right answers, no validation >> tools. Just whatever seems reasonable to a human eye until such time >> we actually decide to pursue publishing the API docs in the >> development manual, if indeed we ever do so at all. >> >> That series sounds like a great opportunity to hash this all out. That >> is when I would like to remove --missing-docstring, too. There will >> absolutely be a "docstring series" in the future, but I am insisting >> stubbornly it happen after strict typing. > > Okay. Nevertheless, I'd prefer a bit more information in the commit > message. Here's my try: > > qapi: Modify docstrings to be sphinx-compatible > > I did not say "sphinx beautiful", just "sphinx compatible". They > will not throw errors when parsed and interpreted as ReST. Finding > a comprehensive style guide for our docstring utilization is left > for another day. > > For now, use field lists recognized by Sphinx autodoc. > FIXME link to their documentation > That I can do -- and I will double down on my IOU for a more formal style guide: https://gitlab.com/jsnow/qemu/-/issues/7 I didn't bother writing it in any of the commits because I felt like it'd get lost there and would be mostly useless; but a .rst doc inside the package folder would be hard to miss. I plan to check in something like ./python/README.rst or ./python/CODING_STYLE.rst to try and formalize a lot of what I am doing here, where it's going to be harder to miss. >> >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> scripts/qapi/gen.py | 6 ++++-- >>>> scripts/qapi/parser.py | 9 +++++---- >>>> 2 files changed, 9 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >>>> index ca66c82b5b8..fc19b2aeb9b 100644 >>>> --- a/scripts/qapi/gen.py >>>> +++ b/scripts/qapi/gen.py >>>> @@ -154,9 +154,11 @@ def _bottom(self): >>>> @contextmanager >>>> def ifcontext(ifcond, *args): >>>> - """A 'with' statement context manager to wrap with start_if()/end_if() >>>> + """ >>>> + A 'with' statement context manager that wraps with `start_if` and `end_if`. >>> Sadly, the fact that start_if() and end_if() are functions isn't >>> immediately obvious anymore. >>> I've seen :func:`start_if` elsewhere. Is this something we should >>> or >>> want to use? >>> >> >> We *could*. >> >> `start_if` relies on the default role, which I have provisionally set >> to "any" here, so this is shorthand for :any:`start_if`. >> >> The :any: role means: "cross-reference any type of thing." If there is >> not exactly one thing that matches, it results in an error during the >> documentation build. >> >> I like this, because it's nice short-hand syntax that I think >> communicates effectively to the reader that this is a symbol of some >> kind without needing a premium of ReST-ese. >> >> CONSTANTS are capitalized, Classes are title cased, and functions are >> lower_case. `lower_case` references can be assumed to be functions, > > `lower_case` could also refer to an attribute, variable, or parameter. > Hm. Attribute yes, actually. variable and parameter no -- sphinx does not presently provide syntax or roles for creating anchors to parameter names or variables, so they are not able to be cross-referenced. Attributes CAN be cross-referenced, but only when they are documented. Another style guide thing: #: x is a number that represents "The Answer". See `Douglas Adams`_. self.x = 42 You can use the special comment form "#:" to add a one-line description of an attribute that Sphinx will pick up. Sphinx skips these attributes otherwise. If you consider them part of the interface of the module, it's maybe a good idea to do this. You can also use docstrings, but the ordering changes: self.x = 42 """x is a number that represents "The Answer". See `Douglas Adams`_. I kind of like the #: form because it announces what follows, but I admit it's a bit of special sphinx magic. >> but I will admit that this is not enforced or necessarily true as we >> add more cross reference types in the future. >> >> (I am trying to add QMP cross-reference syntax!) >> >> I still prefer `start_if` to :func:`start_if` simply because it's less >> markup and is easier to read in plaintext contexts. You're right, it >> doesn't look like a function anymore. > > Yes, :func:`start_if` is rather heavy. I asked because I wanted to > understand what :func: buys us. Not meant as endorsement. > It specifically targets only cross-references of that exact type. In the case that the :any: reference is ambiguous, :func: is the disambiguation. > GDK-Doc seems smart enough to recognize start_if(). Sphinx isn't, > because it's built around reST syntax. We put our money on the Sphinx > horse, so... > >> I'm not sure if another annotations would work -- `start_if`() or >> `start_if()`. Both seem kind of clunky to me, to be honest. Personal >> feeling is "not really worth the hassle." > > You later reported the latter works. > > I prefer `start_if()` to `start_if`. Matter of taste. > Change made. >> >>>> - *args: any number of QAPIGenCCode >>>> + :param ifcond: List of conditionals >>>> + :param args: any number of `QAPIGenCCode`. >>>> Example:: >>>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >>>> index 9d1a3e2eea9..02983979965 100644 >>>> --- a/scripts/qapi/parser.py >>>> +++ b/scripts/qapi/parser.py >>>> @@ -381,10 +381,11 @@ def append(self, line): >>>> The way that the line is dealt with depends on which >>>> part of >>>> the documentation we're parsing right now: >>>> - * The body section: ._append_line is ._append_body_line >>>> - * An argument section: ._append_line is ._append_args_line >>>> - * A features section: ._append_line is ._append_features_line >>>> - * An additional section: ._append_line is ._append_various_line >>>> + >>>> + * The body section: ._append_line is ._append_body_line >>>> + * An argument section: ._append_line is ._append_args_line >>>> + * A features section: ._append_line is ._append_features_line >>>> + * An additional section: ._append_line is ._append_various_line >>>> """ >>>> line = line[1:] >>>> if not line: >>> I understand why you insert a blank line (reST wants blank lines >>> around >>> lists), I don't understand why you indent. Can you explain? >> >> I was mistaken about it needing the indent! > > Easy enough to tidy up :) > Already done!
On 9/30/20 3:52 PM, Eduardo Habkost wrote: > On Wed, Sep 30, 2020 at 02:58:04PM -0400, John Snow wrote: >> On 9/30/20 2:39 PM, Eduardo Habkost wrote: >>> On Wed, Sep 30, 2020 at 12:31:45AM -0400, John Snow wrote: >>>> This replaces _make_tree with Node.__init__(), effectively. By creating >>>> it as a generic container, we can more accurately describe the exact >>>> nature of this particular Node. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> scripts/qapi/introspect.py | 77 +++++++++++++++++++------------------- >>>> 1 file changed, 38 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py >>>> index 43b6ba5df1f..86286e755ca 100644 >>>> --- a/scripts/qapi/introspect.py >>>> +++ b/scripts/qapi/introspect.py >>>> @@ -12,11 +12,12 @@ >>>> from typing import ( >>>> Dict, >>>> + Generic, >>>> + Iterable, >>>> List, >>>> Optional, >>>> Sequence, >>>> - Tuple, >>>> - Union, >>>> + TypeVar, >>>> ) >>>> from .common import ( >>>> @@ -43,42 +44,42 @@ >>>> # The correct type for TreeNode is actually: >>>> -# Union[AnnotatedNode, List[TreeNode], Dict[str, TreeNode], str, bool] >>>> +# Union[Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool] >>>> # but mypy does not support recursive types yet. >>>> TreeNode = object >>>> +_NodeType = TypeVar('_NodeType', bound=TreeNode) >>>> _DObject = Dict[str, object] >>>> -Extra = Dict[str, object] >>>> -AnnotatedNode = Tuple[TreeNode, Extra] >>> >>> Do you have plans to make Node replace TreeNode completely? >>> >>> I'd understand this patch as a means to reach that goal, but I'm >>> not sure the intermediate state of having both Node and TreeNode >>> types (that can be confused with each other) is desirable. >>> >> >> The problem is that _tree_to_qlit still accepts a broad array of types. The >> "TreeNode" comment above explains that those types are: >> >> Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool >> >> Three are containers, two are leaf values. >> of the containers, the Node container is special in that it houses >> explicitly one of the four other types (but never itself.) >> >> Even if I somehow always enforced Node[T] heading into _tree_to_qlit, I >> would still need to describe what 'T' is, which is another recursive type >> that I cannot exactly describe with mypy's current descriptive power: >> >> INNER_TYPE = List[Node[INNER_TYPE]], Dict[str, Node[INNER_TYPE]], str, bool >> >> And that's not really a huge win, or indeed any different to the existing >> TreeNode being an "object". >> >> So ... no, I felt like I was going to stop here, where we have >> fundamentally: >> >> 1. Undecorated nodes (list, dict, str, bool) ("TreeNode") >> 2. Decorated nodes (Node[T]) ("Node") >> >> which leads to the question: Why bother swapping Tuple for Node at that >> point? >> >> My answer is simply that having a strong type name allows us to distinguish >> this from garden-variety Tuples that might sneak in for other reasons in >> other data types. > > Would: > AnnotatedNode = NewType('AnnotatedNode', Tuple[TreeNode, Extra]) > be enough, then? > I don't think so, because the runtime check still checks for tuple. I like the consistency and simplicity of a named type. >> >> Maybe we want a different nomenclature though, like Node vs AnnotatedNode? > > Yes, I believe having a more explicit name would be better. > I give up on introspect.py; I'm dropping it from my series. I cannot possibly justify another single second spent here. --js
John Snow <jsnow@redhat.com> writes: > On 10/1/20 4:52 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 9/30/20 4:47 AM, Markus Armbruster wrote: >>>> John Snow <jsnow@redhat.com> writes: >>>> >>>>> I did not say "sphinx beautiful", just "sphinx compatible". They will >>>>> not throw errors when parsed and interpreted as ReST. >>>> "Bang on the keyboard until Sphinx doesn't throw errors anymore" >>>> might >>>> be good enough for a certain kind of mathematician, but a constructive >>>> solution needs a bit more direction. Is there a specification to >>>> follow? Other useful resources? >>>> >>> >>> I don't know if you are asking this question rhetorically, or in good faith. >> I ask to make sure I understand goals and limitations of your doc >> string >> work in this series. >> Also, even a passing to Sphinx becomes more useful when accompanied >> by a >> link to relevant documentation. >> >>> Let me preface this by saying: This series, and these 119 patches, are >>> not about finding a style guide for our docstring utilization or about >>> proposing one. It is also not about rigorously adding such >>> documentation or about finding ways to meaningfully publish it with >>> e.g. Sphinx, or the styling of such pages. >>> >>> Why bother to add docstrings at all, then? Because I needed them for >>> my own sake when learning this code and I felt it would be a waste to >>> just delete them, and I am of sound mind and able body and believe >>> that some documentation was better than none. They are useful even >>> just as plaintext. >>> >>> Having said that, let's explore the actual style I tend to use. >>> >>> I mentioned before in response to a review comment that there isn't >>> really any standard for docstrings. There are a few competing >>> "styles", but none of which are able to be programmatically checked >>> and validated. >>> >>> The primary guide for docstrings is PEP 257, of which I follow some >>> but not all of the recommendations. >>> >>> https://www.python.org/dev/peps/pep-0257/ >> >> I find PEP 257 frustrating. It leaves me with more questions than >> answers. > > Yeah, sorry. That's what we're dealing with. It's very open-ended. > >>> In general, >>> >>> - Always use triple-double quotes (unenforced) >>> - Modules, classes, and functions should have docstrings (pylint) >>> - No empty lines before or after the docstring (unenforced) >>> - Multi-line docstrings should take the form (unenforced): >>> >>> """ >>> single-line summary of function. >>> >>> Additional prose if needed describing the function. >>> >>> :param x: Input such that blah blah. >>> :raises y: When input ``x`` is unsuitable because blah blah. >>> :returns: A value that blah blah. >> This paragraph is already not PEP 257. >> > > Right -- well, it isn't NOT PEP 257 either. It just suggests you have > to describe these features, but it doesn't say HOW. Yep. Frustrating. >>> """ >>> >>> PEP257 suggests a form where the single-line summary appears on the >>> same line as the opening triple quotes. I don't like this, and prefer >>> symmetry. PEP257 *also* suggests that writing it my way is equivalent >>> to their way, because any docstring processor should trim the first >>> line. I take this as tacit admission that my way is acceptable and has >>> merit. >> I prefer the symmetric form myself. >> >>> What about the syntax or markup inside the docstring itself? there is >>> *absolutely no standard*, but Sphinx autodoc recognizes a few field >>> lists as significant in its parsing, so I prefer using them: >> >> Doc link? > > Hard to search for in my opinion; More reason to provide a link! > you want to search for "sphinx > python domain", and then click on "field lists" on the sidebar. > > https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists > > It has a special understanding for: > > param/parameter/arg/argument/key/keyword: I prefer "param" > here. Possibly key/keyword if we use a **kwargs form with a key that > we specially recognize, but I've not tested that yet. I know pycharm > understands "param" in a semantic way, and that's been good enough for me. > > type: Defines the type of a parameter. In my opinion, do not use > this. Let native type hints do the lifting. Agree. > raises/raise/except/exception: I prefer "raises". "raises ErrorType > when...." is a good sentence. > > var/ivar/cvar: Describes a variable, presumably in the body of the > function below. I've never used this, I always describe it in prose > instead. > > vartype: Defines a type for a variable; I would again defer to the > native type system instead now. > > returns/return: I prefer "returns" for grammatical reasons > again. ("Returns such-and-such when...") "Return such-and-such when..." is just as correct: imperative mood. I prefer imperative mood for function contracts. > rtype: again, type information. Don't use. > > meta: For directives to sphinx, e.g. :meta private: or :meta public: > to toggle the visibility class from its default. I don't use this. > > > None of these are validated or checked in any meaningful way; you can > use arbitrarily field lists (and I do in a few places!) to define your > own terms and so on. > > > (I would like to improve autodoc in the future to validate your > docstrings such that you can enforce :param:, :raises: and :return: > and it uses the type hints and introspection information to raise an > error when you make an obvious mistake. I am not there yet, but I am > using Peter Maydell's work to help inform how I might write such an > extension to autodoc. This work is not critical, but it will likely > occur upstream, outside of the QEMU context because I believe this is > a good thing to do for the ecosystem in general, to allow autodoc to > function slightly more like e.g. Doxygen does.) Sounds useful, but yes, it's clearly outside QEMU context. >>> :param x: Denotes the parameter X. Do not use type information in the >>> string, we rely on mypy for that now. >>> >>> :raises y: explains a case in which an Exception type y may be raised >>> either directly by this code or anticipated to be allowed to be raised >>> by a helper call. (There's no standard here that I am aware of. I use >>> my judgment. Always document direct raise calls, but use your judgment >>> for sub-calls.) >>> >>> :returns: explains the semantics of the return value. >>> >>> That said, literally any sphinx/ReST markup is OK as long as it passes >>> make sphinxdocs. Some sphinx markup is prohibited, like adding new >>> full-throated sections. You can use arbitrary field lists, definition >>> lists, pre-formatted text, examples, code blocks, whatever. >>> >>> In general, you are not going to find the kind of semantic validation >>> you want to ensure that the parameter names are correct, or that you >>> spelled :param: right, or that you didn't miss a parameter or an >>> exception. None of that tooling exists for Python. >>> >>> Thus, it's all rather subjective. No right answers, no validation >>> tools. Just whatever seems reasonable to a human eye until such time >>> we actually decide to pursue publishing the API docs in the >>> development manual, if indeed we ever do so at all. >>> >>> That series sounds like a great opportunity to hash this all out. That >>> is when I would like to remove --missing-docstring, too. There will >>> absolutely be a "docstring series" in the future, but I am insisting >>> stubbornly it happen after strict typing. >> >> Okay. Nevertheless, I'd prefer a bit more information in the commit >> message. Here's my try: >> >> qapi: Modify docstrings to be sphinx-compatible >> >> I did not say "sphinx beautiful", just "sphinx compatible". They >> will not throw errors when parsed and interpreted as ReST. Finding >> a comprehensive style guide for our docstring utilization is left >> for another day. >> >> For now, use field lists recognized by Sphinx autodoc. >> FIXME link to their documentation > > That I can do -- and I will double down on my IOU for a more formal > style guide: https://gitlab.com/jsnow/qemu/-/issues/7 > > I didn't bother writing it in any of the commits because I felt like > it'd get lost there and would be mostly useless; but a .rst doc inside > the package folder would be hard to miss. > > I plan to check in something like ./python/README.rst or > ./python/CODING_STYLE.rst to try and formalize a lot of what I am > doing here, where it's going to be harder to miss. Makes sense. >>>>> >>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>> --- >>>>> scripts/qapi/gen.py | 6 ++++-- >>>>> scripts/qapi/parser.py | 9 +++++---- >>>>> 2 files changed, 9 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >>>>> index ca66c82b5b8..fc19b2aeb9b 100644 >>>>> --- a/scripts/qapi/gen.py >>>>> +++ b/scripts/qapi/gen.py >>>>> @@ -154,9 +154,11 @@ def _bottom(self): >>>>> @contextmanager >>>>> def ifcontext(ifcond, *args): >>>>> - """A 'with' statement context manager to wrap with start_if()/end_if() >>>>> + """ >>>>> + A 'with' statement context manager that wraps with `start_if` and `end_if`. >>>> Sadly, the fact that start_if() and end_if() are functions isn't >>>> immediately obvious anymore. >>>> I've seen :func:`start_if` elsewhere. Is this something we should >>>> or >>>> want to use? >>>> >>> >>> We *could*. >>> >>> `start_if` relies on the default role, which I have provisionally set >>> to "any" here, so this is shorthand for :any:`start_if`. >>> >>> The :any: role means: "cross-reference any type of thing." If there is >>> not exactly one thing that matches, it results in an error during the >>> documentation build. >>> >>> I like this, because it's nice short-hand syntax that I think >>> communicates effectively to the reader that this is a symbol of some >>> kind without needing a premium of ReST-ese. >>> >>> CONSTANTS are capitalized, Classes are title cased, and functions are >>> lower_case. `lower_case` references can be assumed to be functions, >> >> `lower_case` could also refer to an attribute, variable, or >> parameter. > > Hm. Attribute yes, actually. variable and parameter no -- sphinx does > not presently provide syntax or roles for creating anchors to > parameter names or variables, so they are not able to be > cross-referenced. How would you mark up variable names in doc strings? Often, their "variableness" is obvious from context, but not always. In C comments, we tend to use @var [*]. > Attributes CAN be cross-referenced, but only when they are documented. > > Another style guide thing: > > #: x is a number that represents "The Answer". See `Douglas Adams`_. > self.x = 42 > > You can use the special comment form "#:" to add a one-line > description of an attribute that Sphinx will pick up. Sphinx skips > these attributes otherwise. If you consider them part of the interface > of the module, it's maybe a good idea to do this. > > You can also use docstrings, but the ordering changes: > > self.x = 42 > """x is a number that represents "The Answer". See `Douglas Adams`_. > > I kind of like the #: form because it announces what follows, but I > admit it's a bit of special sphinx magic. Are both equally available in Python IDEs and in interactive Python? >>> but I will admit that this is not enforced or necessarily true as we >>> add more cross reference types in the future. >>> >>> (I am trying to add QMP cross-reference syntax!) >>> >>> I still prefer `start_if` to :func:`start_if` simply because it's less >>> markup and is easier to read in plaintext contexts. You're right, it >>> doesn't look like a function anymore. >> Yes, :func:`start_if` is rather heavy. I asked because I wanted to >> understand what :func: buys us. Not meant as endorsement. >> > > It specifically targets only cross-references of that exact type. In > the case that the :any: reference is ambiguous, :func: is the > disambiguation. > >> GDK-Doc seems smart enough to recognize start_if(). Sphinx isn't, >> because it's built around reST syntax. We put our money on the Sphinx >> horse, so... >> >>> I'm not sure if another annotations would work -- `start_if`() or >>> `start_if()`. Both seem kind of clunky to me, to be honest. Personal >>> feeling is "not really worth the hassle." >> >> You later reported the latter works. >> I prefer `start_if()` to `start_if`. Matter of taste. > > Change made. Thanks! >>>>> - *args: any number of QAPIGenCCode >>>>> + :param ifcond: List of conditionals >>>>> + :param args: any number of `QAPIGenCCode`. >>>>> Example:: >>>>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >>>>> index 9d1a3e2eea9..02983979965 100644 >>>>> --- a/scripts/qapi/parser.py >>>>> +++ b/scripts/qapi/parser.py >>>>> @@ -381,10 +381,11 @@ def append(self, line): >>>>> The way that the line is dealt with depends on which >>>>> part of >>>>> the documentation we're parsing right now: >>>>> - * The body section: ._append_line is ._append_body_line >>>>> - * An argument section: ._append_line is ._append_args_line >>>>> - * A features section: ._append_line is ._append_features_line >>>>> - * An additional section: ._append_line is ._append_various_line >>>>> + >>>>> + * The body section: ._append_line is ._append_body_line >>>>> + * An argument section: ._append_line is ._append_args_line >>>>> + * A features section: ._append_line is ._append_features_line >>>>> + * An additional section: ._append_line is ._append_various_line >>>>> """ >>>>> line = line[1:] >>>>> if not line: >>>> I understand why you insert a blank line (reST wants blank lines >>>> around >>>> lists), I don't understand why you indent. Can you explain? >>> >>> I was mistaken about it needing the indent! >> Easy enough to tidy up :) >> > > Already done! Thanks again! [*] GTK-Doc says @var is just for parameters, but since it offers nothing for variables, we sometimes use it for variables as well.
On 10/2/20 5:19 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 10/1/20 4:52 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >>> >>>> On 9/30/20 4:47 AM, Markus Armbruster wrote: >>>>> John Snow <jsnow@redhat.com> writes: >>>>> >>>>>> I did not say "sphinx beautiful", just "sphinx compatible". They will >>>>>> not throw errors when parsed and interpreted as ReST. >>>>> "Bang on the keyboard until Sphinx doesn't throw errors anymore" >>>>> might >>>>> be good enough for a certain kind of mathematician, but a constructive >>>>> solution needs a bit more direction. Is there a specification to >>>>> follow? Other useful resources? >>>>> >>>> >>>> I don't know if you are asking this question rhetorically, or in good faith. >>> I ask to make sure I understand goals and limitations of your doc >>> string >>> work in this series. >>> Also, even a passing to Sphinx becomes more useful when accompanied >>> by a >>> link to relevant documentation. >>> >>>> Let me preface this by saying: This series, and these 119 patches, are >>>> not about finding a style guide for our docstring utilization or about >>>> proposing one. It is also not about rigorously adding such >>>> documentation or about finding ways to meaningfully publish it with >>>> e.g. Sphinx, or the styling of such pages. >>>> >>>> Why bother to add docstrings at all, then? Because I needed them for >>>> my own sake when learning this code and I felt it would be a waste to >>>> just delete them, and I am of sound mind and able body and believe >>>> that some documentation was better than none. They are useful even >>>> just as plaintext. >>>> >>>> Having said that, let's explore the actual style I tend to use. >>>> >>>> I mentioned before in response to a review comment that there isn't >>>> really any standard for docstrings. There are a few competing >>>> "styles", but none of which are able to be programmatically checked >>>> and validated. >>>> >>>> The primary guide for docstrings is PEP 257, of which I follow some >>>> but not all of the recommendations. >>>> >>>> https://www.python.org/dev/peps/pep-0257/ >>> >>> I find PEP 257 frustrating. It leaves me with more questions than >>> answers. >> >> Yeah, sorry. That's what we're dealing with. It's very open-ended. >> >>>> In general, >>>> >>>> - Always use triple-double quotes (unenforced) >>>> - Modules, classes, and functions should have docstrings (pylint) >>>> - No empty lines before or after the docstring (unenforced) >>>> - Multi-line docstrings should take the form (unenforced): >>>> >>>> """ >>>> single-line summary of function. >>>> >>>> Additional prose if needed describing the function. >>>> >>>> :param x: Input such that blah blah. >>>> :raises y: When input ``x`` is unsuitable because blah blah. >>>> :returns: A value that blah blah. >>> This paragraph is already not PEP 257. >>> >> >> Right -- well, it isn't NOT PEP 257 either. It just suggests you have >> to describe these features, but it doesn't say HOW. > > Yep. Frustrating. > >>>> """ >>>> >>>> PEP257 suggests a form where the single-line summary appears on the >>>> same line as the opening triple quotes. I don't like this, and prefer >>>> symmetry. PEP257 *also* suggests that writing it my way is equivalent >>>> to their way, because any docstring processor should trim the first >>>> line. I take this as tacit admission that my way is acceptable and has >>>> merit. >>> I prefer the symmetric form myself. >>> >>>> What about the syntax or markup inside the docstring itself? there is >>>> *absolutely no standard*, but Sphinx autodoc recognizes a few field >>>> lists as significant in its parsing, so I prefer using them: >>> >>> Doc link? >> >> Hard to search for in my opinion; > > More reason to provide a link! > Yup. I took a stab at it in a new commit message. >> you want to search for "sphinx >> python domain", and then click on "field lists" on the sidebar. >> >> https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists >> >> It has a special understanding for: >> >> param/parameter/arg/argument/key/keyword: I prefer "param" >> here. Possibly key/keyword if we use a **kwargs form with a key that >> we specially recognize, but I've not tested that yet. I know pycharm >> understands "param" in a semantic way, and that's been good enough for me. >> >> type: Defines the type of a parameter. In my opinion, do not use >> this. Let native type hints do the lifting. > > Agree. > >> raises/raise/except/exception: I prefer "raises". "raises ErrorType >> when...." is a good sentence. >> >> var/ivar/cvar: Describes a variable, presumably in the body of the >> function below. I've never used this, I always describe it in prose >> instead. >> >> vartype: Defines a type for a variable; I would again defer to the >> native type system instead now. >> >> returns/return: I prefer "returns" for grammatical reasons >> again. ("Returns such-and-such when...") > > "Return such-and-such when..." is just as correct: imperative mood. I > prefer imperative mood for function contracts. > I think there's some other style guide I saw that prefers this too. I think I casually prefer "raises" and "returns", but "raise X when" and "return Y such that" both make sense, too. Not something I'll go to fight and die for, but it's probably the case that I have already been using the other form. I can fix these alongside a style guide if you'd like, but I'd ask for your lenience to allow me do that as a follow-up in a series that I won't mind spending more time in review to perfect our style. >> rtype: again, type information. Don't use. >> >> meta: For directives to sphinx, e.g. :meta private: or :meta public: >> to toggle the visibility class from its default. I don't use this. >> >> >> None of these are validated or checked in any meaningful way; you can >> use arbitrarily field lists (and I do in a few places!) to define your >> own terms and so on. >> >> >> (I would like to improve autodoc in the future to validate your >> docstrings such that you can enforce :param:, :raises: and :return: >> and it uses the type hints and introspection information to raise an >> error when you make an obvious mistake. I am not there yet, but I am >> using Peter Maydell's work to help inform how I might write such an >> extension to autodoc. This work is not critical, but it will likely >> occur upstream, outside of the QEMU context because I believe this is >> a good thing to do for the ecosystem in general, to allow autodoc to >> function slightly more like e.g. Doxygen does.) > > Sounds useful, but yes, it's clearly outside QEMU context. > >>>> :param x: Denotes the parameter X. Do not use type information in the >>>> string, we rely on mypy for that now. >>>> >>>> :raises y: explains a case in which an Exception type y may be raised >>>> either directly by this code or anticipated to be allowed to be raised >>>> by a helper call. (There's no standard here that I am aware of. I use >>>> my judgment. Always document direct raise calls, but use your judgment >>>> for sub-calls.) >>>> >>>> :returns: explains the semantics of the return value. >>>> >>>> That said, literally any sphinx/ReST markup is OK as long as it passes >>>> make sphinxdocs. Some sphinx markup is prohibited, like adding new >>>> full-throated sections. You can use arbitrary field lists, definition >>>> lists, pre-formatted text, examples, code blocks, whatever. >>>> >>>> In general, you are not going to find the kind of semantic validation >>>> you want to ensure that the parameter names are correct, or that you >>>> spelled :param: right, or that you didn't miss a parameter or an >>>> exception. None of that tooling exists for Python. >>>> >>>> Thus, it's all rather subjective. No right answers, no validation >>>> tools. Just whatever seems reasonable to a human eye until such time >>>> we actually decide to pursue publishing the API docs in the >>>> development manual, if indeed we ever do so at all. >>>> >>>> That series sounds like a great opportunity to hash this all out. That >>>> is when I would like to remove --missing-docstring, too. There will >>>> absolutely be a "docstring series" in the future, but I am insisting >>>> stubbornly it happen after strict typing. >>> >>> Okay. Nevertheless, I'd prefer a bit more information in the commit >>> message. Here's my try: >>> >>> qapi: Modify docstrings to be sphinx-compatible >>> >>> I did not say "sphinx beautiful", just "sphinx compatible". They >>> will not throw errors when parsed and interpreted as ReST. Finding >>> a comprehensive style guide for our docstring utilization is left >>> for another day. >>> >>> For now, use field lists recognized by Sphinx autodoc. >>> FIXME link to their documentation >> >> That I can do -- and I will double down on my IOU for a more formal >> style guide: https://gitlab.com/jsnow/qemu/-/issues/7 >> >> I didn't bother writing it in any of the commits because I felt like >> it'd get lost there and would be mostly useless; but a .rst doc inside >> the package folder would be hard to miss. >> >> I plan to check in something like ./python/README.rst or >> ./python/CODING_STYLE.rst to try and formalize a lot of what I am >> doing here, where it's going to be harder to miss. > > Makes sense. > >>>>>> >>>>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>>>> --- >>>>>> scripts/qapi/gen.py | 6 ++++-- >>>>>> scripts/qapi/parser.py | 9 +++++---- >>>>>> 2 files changed, 9 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >>>>>> index ca66c82b5b8..fc19b2aeb9b 100644 >>>>>> --- a/scripts/qapi/gen.py >>>>>> +++ b/scripts/qapi/gen.py >>>>>> @@ -154,9 +154,11 @@ def _bottom(self): >>>>>> @contextmanager >>>>>> def ifcontext(ifcond, *args): >>>>>> - """A 'with' statement context manager to wrap with start_if()/end_if() >>>>>> + """ >>>>>> + A 'with' statement context manager that wraps with `start_if` and `end_if`. >>>>> Sadly, the fact that start_if() and end_if() are functions isn't >>>>> immediately obvious anymore. >>>>> I've seen :func:`start_if` elsewhere. Is this something we should >>>>> or >>>>> want to use? >>>>> >>>> >>>> We *could*. >>>> >>>> `start_if` relies on the default role, which I have provisionally set >>>> to "any" here, so this is shorthand for :any:`start_if`. >>>> >>>> The :any: role means: "cross-reference any type of thing." If there is >>>> not exactly one thing that matches, it results in an error during the >>>> documentation build. >>>> >>>> I like this, because it's nice short-hand syntax that I think >>>> communicates effectively to the reader that this is a symbol of some >>>> kind without needing a premium of ReST-ese. >>>> >>>> CONSTANTS are capitalized, Classes are title cased, and functions are >>>> lower_case. `lower_case` references can be assumed to be functions, >>> >>> `lower_case` could also refer to an attribute, variable, or >>> parameter. >> >> Hm. Attribute yes, actually. variable and parameter no -- sphinx does >> not presently provide syntax or roles for creating anchors to >> parameter names or variables, so they are not able to be >> cross-referenced. > > How would you mark up variable names in doc strings? Often, their > "variableness" is obvious from context, but not always. In C comments, > we tend to use @var [*]. > I have been using ``var`` so far, to render it in a preformatted monowidth style. It doesn't cross-reference anything and it is no different from other pre-formatted literal blocks. I use the same markup for Python types; ``str`` and ``List[str]`` and so on -- other literal values like ``True``, ``False``, and ``"string_literal"``. We could look into creating markup for it, like :v:`var` or something, to which we apply rendering logic to distinguish them, but that's beyond my means to do quickly at the moment. I can add it to my list to investigate ("someday"). Also possible that we might look at autodoc extensions to generate cross-references for variable names (by modifying handling for :param:?), but I also don't know if rst/sphinx references can be "scoped"? Variables and parameters will collide far more often than function/class/module names. If you could do, say, `funcname.varname` in cases where the variable was ambiguous and it jumped to the sphinx docs where that variable/parameter was described, that'd be hot. But it's not something that's tractable to me right now, sorry. >> Attributes CAN be cross-referenced, but only when they are documented. >> >> Another style guide thing: >> >> #: x is a number that represents "The Answer". See `Douglas Adams`_. >> self.x = 42 >> >> You can use the special comment form "#:" to add a one-line >> description of an attribute that Sphinx will pick up. Sphinx skips >> these attributes otherwise. If you consider them part of the interface >> of the module, it's maybe a good idea to do this. >> >> You can also use docstrings, but the ordering changes: >> >> self.x = 42 >> """x is a number that represents "The Answer". See `Douglas Adams`_. >> >> I kind of like the #: form because it announces what follows, but I >> admit it's a bit of special sphinx magic. > > Are both equally available in Python IDEs and in interactive Python? > Docstrings are only recognized as special for modules, classes and functions. I do not think Python recognizes docstrings for attributes at all, actually. >>> help(qapi.common) is going to show you: - the description (docstring, comments) for the module - the classes defined in it (Indentation), - functions and their docstrings (c_enum_const, c_fname, c_name, ...) - "Data": (EATSPACE, POINTER_SUFFIX, indent) I commented EATSPACE in my patches because I reference it from somewhere else. Either style does not show up in help(qapi.common). qapi.common.EATSPACE does have a __doc__ attribute, but it's actually the doc attribute for `str()`! Not what we wanted at all. So either style is invisible at runtime. Sad. We can also prefer to use @property style functions which can receive docstrings -- but this only works for classes! I don't think there's a standard way to define "properties" for modules exactly as such. That means that module-level data (constants, globals, etc.) don't really have a great way to be documented. The best place is likely actually in the module docstring itself, but you cannot cross-reference such a location with Sphinx, I think. Needs more research, but again I plead that this is good fodder for a later series when we attempt to calcify a standard. >>>> but I will admit that this is not enforced or necessarily true as we >>>> add more cross reference types in the future. >>>> >>>> (I am trying to add QMP cross-reference syntax!) >>>> >>>> I still prefer `start_if` to :func:`start_if` simply because it's less >>>> markup and is easier to read in plaintext contexts. You're right, it >>>> doesn't look like a function anymore. >>> Yes, :func:`start_if` is rather heavy. I asked because I wanted to >>> understand what :func: buys us. Not meant as endorsement. >>> >> >> It specifically targets only cross-references of that exact type. In >> the case that the :any: reference is ambiguous, :func: is the >> disambiguation. >> >>> GDK-Doc seems smart enough to recognize start_if(). Sphinx isn't, >>> because it's built around reST syntax. We put our money on the Sphinx >>> horse, so... >>> >>>> I'm not sure if another annotations would work -- `start_if`() or >>>> `start_if()`. Both seem kind of clunky to me, to be honest. Personal >>>> feeling is "not really worth the hassle." >>> >>> You later reported the latter works. >>> I prefer `start_if()` to `start_if`. Matter of taste. >> >> Change made. > > Thanks! > >>>>>> - *args: any number of QAPIGenCCode >>>>>> + :param ifcond: List of conditionals >>>>>> + :param args: any number of `QAPIGenCCode`. >>>>>> Example:: >>>>>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >>>>>> index 9d1a3e2eea9..02983979965 100644 >>>>>> --- a/scripts/qapi/parser.py >>>>>> +++ b/scripts/qapi/parser.py >>>>>> @@ -381,10 +381,11 @@ def append(self, line): >>>>>> The way that the line is dealt with depends on which >>>>>> part of >>>>>> the documentation we're parsing right now: >>>>>> - * The body section: ._append_line is ._append_body_line >>>>>> - * An argument section: ._append_line is ._append_args_line >>>>>> - * A features section: ._append_line is ._append_features_line >>>>>> - * An additional section: ._append_line is ._append_various_line >>>>>> + >>>>>> + * The body section: ._append_line is ._append_body_line >>>>>> + * An argument section: ._append_line is ._append_args_line >>>>>> + * A features section: ._append_line is ._append_features_line >>>>>> + * An additional section: ._append_line is ._append_various_line >>>>>> """ >>>>>> line = line[1:] >>>>>> if not line: >>>>> I understand why you insert a blank line (reST wants blank lines >>>>> around >>>>> lists), I don't understand why you indent. Can you explain? >>>> >>>> I was mistaken about it needing the indent! >>> Easy enough to tidy up :) >>> >> >> Already done! > > Thanks again! > > > [*] GTK-Doc says @var is just for parameters, but since it offers > nothing for variables, we sometimes use it for variables as well. >
Based-on: <20200925162316.21205-1-peter.maydell@linaro.org> Hi, this series adds static type hints to the QAPI module. This is part one! Part 1: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt1 Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6 - Requires Python 3.6+ - Requires mypy 0.770 or newer (for type analysis only) - Requires pylint 2.6.0 or newer (for lint checking only) In general, this series tackles the cleanup of one individual QAPI module at a time. Once it passes pylint or mypy checks, those checks are enabled for that file. Type hints are added in patches that add *only* type hints and change no other behavior. Any necessary changes to behavior to accommodate typing are split out into their own tiny patches. Notes: - `make sphinxdocs` should work on every commit. It begins to include docstring content after the DO-NOT-MERGE patch 5. - After patch 11, `isort -c` should pass 100% on this and every future commit. - After patch 12, `flake8 qapi/` should pass 100% on this and every future commit. - After patch 13, `pylint --rcfile=3Dqapi/pylintrc qapi/` should pass 100% on this and every future commit. - After patch 22, `mypy --config-file=3Dqapi/mypy.ini qapi/` should pass 100% on this and every future commit. V4: - Rebase on Peter Maydell's work; - 05: Context differences from Peter Maydell's work - 06: Remove qapi.doc - 07: Remove qapi.doc, remove superfluous "if match" - 09: Remove qapi.doc - 13: remove qapi.doc - 18: remove qapi.doc - 22: remove qapi.doc - 31: remove QAPIGenDoc changes - Minor adjustments from list feedback; - 01: More backticks for QAPI json files, now that they are in Sphinx-exe - 02: Use :ref: role for the `docker-ref` cross-reference - 04: Remove doc.py work changes; add references for start_if/end_if - 10: Changes to accommodate isort - 11: Added lines_after_imports=3D2 - 16: isort whitespace changes - 24: Take Markus's docstring phrasing - 34: add comment explaining os.open() - 37: isort differences Status: (This is my stgit summary with reviewer tags visible.) [01] do-not-merge-docs-replace # [02] docs-repair-broken-references # [03] docs-sphinx-change-default # [04] qapi-modify-docstrings-to-be # [05] do-not-merge-docs-enable # [06] qapi-gen-separate-arg-parsing # [SOB] JS [RB] CR,EH [TB] CR [07] qapi-move-generator-entrypoint # [SOB] JS [RB] CR,EH [TB] CR [08] do-not-merge-more-apidoc # [09] qapi-prefer-explicit-relative # [SOB] JS [RB] CR,EH [10] qapi-remove-wildcard-includes # [SOB] JS [RB] CR,EH [11] qapi-enforce-import-order # [SOB] JS [RB] CR [TB] CR [12] qapi-delint-using-flake8 # [SOB] JS [RB] CR,EH [13] qapi-add-pylintrc # [SOB] JS [TB] CR,EH [RB] CR [14] qapi-common-py-remove-python # [SOB] JS [RB] CR,EH [15] qapi-common-add-indent-manager # [SOB] JS [RB] CR,EH [16] qapi-common-py-delint-with # [SOB] JS [RB] CR,EH [17] replace-c-by-char # [SOB] JS [RB] CR,EH [18] qapi-common-py-check-with # [SOB] JS [RB] CR [TB] CR,EH [19] qapi-common-py-add-notational # [SOB] JS [RB] CR,EH [20] qapi-common-move-comments-into # [SOB] JS [RB] CR,EH [21] qapi-split-build_params-into # [SOB] JS [RB] CR,EH [22] qapi-establish-mypy-type # [SOB] JS [TB] CR,EH [RB] CR [23] qapi-events-py-add-notational # [SOB] JS [RB] CR,EH [24] qapi-events-move-comments-into # [SOB] JS [RB] CR,EH [25] qapi-commands-py-don-t-re-bind # [SOB] JS [RB] CR,EH [26] qapi-commands-py-add # [SOB] JS [RB] CR,EH [27] qapi-commands-py-enable # [SOB] JS [RB] CR,EH [28] qapi-source-py-add-notational # [SOB] JS [RB] CR,EH [TB] CR [29] qapi-source-py-delint-with # [SOB] JS [RB] CR,EH [TB] CR [30] qapi-gen-py-fix-edge-case-of # [SOB] JS [RB] CR [31] qapi-gen-py-add-notational # [SOB] JS [RB] CR,EH [32] qapi-gen-py-enable-checking # [SOB] JS [RB] CR,EH [TB] CR [33] qapi-gen-py-remove-unused # [SOB] JS [RB] CR,EH [34] qapi-gen-py-update-write-to-be # [SOB] JS [RB] CR,EH [35] qapi-gen-py-delint-with-pylint # [SOB] JS [RB] CR,EH [36] qapi-introspect-py-assert-obj # [37] qapi-introspect-py-create-a # [SOB] EH,JS [38] qapi-introspect-py-add # [39] qapi-introspect-py-unify # [40] qapi-introspect-py-replace # [41] qapi-introspect-py-create-a-0 # [42] qapi-types-py-add-type-hint # [SOB] JS [RB] CR,EH [43] qapi-types-py-remove-one # [SOB] JS [RB] CR,EH [44] qapi-visit-py-assert # [SOB] JS [RB] CR,EH [45] qapi-visit-py-remove-unused # [SOB] JS [RB] CR,EH [TB] CR [46] qapi-visit-py-add-notational # [SOB] JS [RB] CR,EH [TB] CR Changelog: (Patches without changes elided) 001/46:[0004] [FC] '[DO-NOT-MERGE] docs: replace single backtick (`) with dou= ble-backtick (``)' 002/46:[0002] [FC] 'docs: repair broken references' 004/46:[0014] [FC] 'qapi: modify docstrings to be sphinx-compatible' 005/46:[0015] [FC] '[DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qa= pi' 006/46:[0004] [FC] 'qapi-gen: Separate arg-parsing from generation' 007/46:[0008] [FC] 'qapi: move generator entrypoint into module' 009/46:[0004] [FC] 'qapi: Prefer explicit relative imports' 010/46:[0006] [FC] 'qapi: Remove wildcard includes' 011/46:[0023] [FC] 'qapi: enforce import order/styling with isort' 013/46:[0001] [FC] 'qapi: add pylintrc' 016/46:[0001] [FC] 'qapi/common.py: delint with pylint' 018/46:[0004] [FC] 'qapi/common.py: check with pylint' 022/46:[0008] [FC] 'qapi: establish mypy type-checking baseline' 024/46:[0004] [FC] 'qapi/events.py: Move comments into docstrings' 031/46:[0003] [FC] 'qapi/gen.py: add type hint annotations' 034/46:[0001] [FC] 'qapi/gen.py: update write() to be more idiomatic' 037/46:[0009] [FC] 'qapi/instrospect.py: add preliminary type hint annotation= s' V3: - Use isort to enforce import consistency - Use sphinx apidoc to check docstring format V2: - Removed Python3.6 patch in favor of Thomas Huth's - Addressed (most) feedback from Markus - Renamed type hint annotation commits Eduardo Habkost (1): qapi/instrospect.py: add preliminary type hint annotations John Snow (45): [DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick (``) docs: repair broken references [DO-NOT-MERGE] docs/sphinx: change default role to "any" qapi: modify docstrings to be sphinx-compatible [DO-NOT-MERGE] docs: enable sphinx-autodoc for scripts/qapi qapi-gen: Separate arg-parsing from generation qapi: move generator entrypoint into module [DO-NOT-MERGE] docs: add scripts/qapi/main to python manual qapi: Prefer explicit relative imports qapi: Remove wildcard includes qapi: enforce import order/styling with isort qapi: delint using flake8 qapi: add pylintrc qapi/common.py: Remove python compatibility workaround qapi/common.py: Add indent manager qapi/common.py: delint with pylint qapi/common.py: Replace one-letter 'c' variable qapi/common.py: check with pylint qapi/common.py: add type hint annotations qapi/common.py: Convert comments into docstrings, and elaborate qapi/common.py: move build_params into gen.py qapi: establish mypy type-checking baseline qapi/events.py: add type hint annotations qapi/events.py: Move comments into docstrings qapi/commands.py: Don't re-bind to variable of different type qapi/commands.py: add type hint annotations qapi/commands.py: enable checking with mypy qapi/source.py: add type hint annotations qapi/source.py: delint with pylint qapi/gen.py: Fix edge-case of _is_user_module qapi/gen.py: add type hint annotations qapi/gen.py: Enable checking with mypy qapi/gen.py: Remove unused parameter qapi/gen.py: update write() to be more idiomatic qapi/gen.py: delint with pylint qapi/introspect.py: assert obj is a dict when features are given qapi/introspect.py: add _gen_features helper qapi/introspect.py: Unify return type of _make_tree() qapi/introspect.py: replace 'extra' dict with 'comment' argument qapi/introspect.py: create a typed 'Node' data structure qapi/types.py: add type hint annotations qapi/types.py: remove one-letter variables qapi/visit.py: assert tag_member contains a QAPISchemaEnumType qapi/visit.py: remove unused parameters from gen_visit_object qapi/visit.py: add type hint annotations docs/conf.py | 6 +- docs/devel/build-system.rst | 118 +++++++------- docs/devel/index.rst | 1 + docs/devel/migration.rst | 59 +++---- docs/devel/multi-thread-tcg.rst | 2 +- docs/devel/python/index.rst | 7 + docs/devel/python/qapi.commands.rst | 7 + docs/devel/python/qapi.common.rst | 7 + docs/devel/python/qapi.error.rst | 7 + docs/devel/python/qapi.events.rst | 7 + docs/devel/python/qapi.expr.rst | 7 + docs/devel/python/qapi.gen.rst | 7 + docs/devel/python/qapi.introspect.rst | 7 + docs/devel/python/qapi.main.rst | 7 + docs/devel/python/qapi.parser.rst | 8 + docs/devel/python/qapi.rst | 26 +++ docs/devel/python/qapi.schema.rst | 7 + docs/devel/python/qapi.source.rst | 7 + docs/devel/python/qapi.types.rst | 7 + docs/devel/python/qapi.visit.rst | 7 + docs/devel/tcg-plugins.rst | 14 +- docs/devel/testing.rst | 4 +- docs/interop/live-block-operations.rst | 4 +- docs/system/arm/cpu-features.rst | 110 ++++++------- docs/system/arm/nuvoton.rst | 2 +- docs/system/s390x/protvirt.rst | 10 +- qapi/block-core.json | 4 +- scripts/qapi-gen.py | 57 ++----- scripts/qapi/.flake8 | 2 + scripts/qapi/.isort.cfg | 7 + scripts/qapi/commands.py | 87 +++++++--- scripts/qapi/common.py | 163 ++++++++++--------- scripts/qapi/events.py | 58 +++++-- scripts/qapi/expr.py | 7 +- scripts/qapi/gen.py | 182 +++++++++++++-------- scripts/qapi/introspect.py | 210 +++++++++++++++++-------- scripts/qapi/main.py | 89 +++++++++++ scripts/qapi/mypy.ini | 25 +++ scripts/qapi/parser.py | 15 +- scripts/qapi/pylintrc | 70 +++++++++ scripts/qapi/schema.py | 35 +++-- scripts/qapi/source.py | 34 ++-- scripts/qapi/types.py | 125 ++++++++++----- scripts/qapi/visit.py | 116 ++++++++++---- 44 files changed, 1175 insertions(+), 566 deletions(-) create mode 100644 docs/devel/python/index.rst create mode 100644 docs/devel/python/qapi.commands.rst create mode 100644 docs/devel/python/qapi.common.rst create mode 100644 docs/devel/python/qapi.error.rst create mode 100644 docs/devel/python/qapi.events.rst create mode 100644 docs/devel/python/qapi.expr.rst create mode 100644 docs/devel/python/qapi.gen.rst create mode 100644 docs/devel/python/qapi.introspect.rst create mode 100644 docs/devel/python/qapi.main.rst create mode 100644 docs/devel/python/qapi.parser.rst create mode 100644 docs/devel/python/qapi.rst create mode 100644 docs/devel/python/qapi.schema.rst create mode 100644 docs/devel/python/qapi.source.rst create mode 100644 docs/devel/python/qapi.types.rst create mode 100644 docs/devel/python/qapi.visit.rst create mode 100644 scripts/qapi/.flake8 create mode 100644 scripts/qapi/.isort.cfg create mode 100644 scripts/qapi/main.py create mode 100644 scripts/qapi/mypy.ini create mode 100644 scripts/qapi/pylintrc --=20 2.26.2