Re: [PATCH v2 15/21] qapi/parser: add docstrings

[John Snow]

On 5/19/21 2:41 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/parser.py | 68 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 68 insertions(+)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index e80e0a7d965..ed543a2b7a4 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -47,7 +47,27 @@ def __init__(self, parser: 'QAPISchemaParser', msg: str):
> >   
> >   
> >   class QAPISchemaParser:
> > +    """
> > +    Performs syntactic parsing of a QAPI schema source file.
>
> "Syntactic parsing" makes me wonder what non-syntactic parsing could be.
>
> Also, PEP 257 wants imperative mood: "Perform X", not "Performs X".
>
> What about a laconic "Parse QAPI schema source"?

Sure. It was just that in an earlier review you seemed keen to spell out 
that this performs the lower-level parsing and not the uh, 
language-level parsing of the QAPI Schema language.

....ehhhhhhh whatever.

"Parse QAPI schema source" it is.

> >   
> > +    Parses a JSON-esque schema file, See qapi-code-gen.txt section
>
> Imperative mood, please.  Period, not comma.
>
> > +    "Schema Syntax" for more information. Grammatical validation
> > +    is handled later by `expr.check_exprs()`.
>
> We could mention the processing of directives.  Perhaps:
>
>         Parse a JSON-esque schema file.  See qapi-code-gen.txt section
>         "Schema Syntax" for the exact syntax.  Also process directives.
>         Grammatical validation is handled later by `expr.check_exprs()`.
>
> What do you think?

    Parse a JSON-esque schema file and process directives.  See 

    qapi-code-gen.txt section "Schema Syntax" for the exact syntax. 

    Grammatical validation is handled later by `expr.check_exprs()`. 


> > +
> > +    :param fname: Source filename.
> > +    :param previously_included:
> > +        The absolute pathnames of previously included source files,
>
> Either file name / filename (either spelling, but let's pick one), or
> pathname, but not both, please.
>
> Possible resolution:
>
>         :param fname: Source file name.
>         :param previously_included:
>             The absolute names of previously included source files,

You got it, boss.

> > +        if being invoked from another parser.
> > +    :param incl_info:
> > +       `QAPISourceInfo` belonging to the parent module.
> > +       ``None`` implies this is the root module.
> > +
> > +    :ivar exprs: Resulting parsed expressions.
> > +    :ivar docs: Resulting parsed documentation blocks.
> > +
> > +    :raise OSError: For problems opening the root schema document.
>
> Hardly matters, but here we go: its both for open() and .read().  We
> could say "reading" instead of "opening".

True enough. Fixed.

> > +    :raise QAPIError: For syntactic or semantic parsing errors.
>
> "Semantic parsing errors" sounds like "triangular squares" :)

I am horrified to learn that words mean things to people. I just pick 
the ones that are the prettiest and cause me to experience dopamine. Am 
I to believe that other people do otherwise?

> I figure you wrote this because we're using both QAPIParseError and
> QAPISemError.  The latter gets raised where we do more than just parse,
> e.g. in directive processing.  It hardly matters, as we don't really
> care for the difference between these error classes anywhere, and
> pragmatically use whatever class is convenient.
>
> Perhaps we should have a single class with multiple constructors
> instead.  Even if yes, not now.

Moving the column tracking stuff directly into QAPISourceInfo would be a 
way to do it. The special constructor there could go away. It could help 
solidify the token :: info correlation.

Then we don't need the two error classes anymore, really. Except for 
semantics, if we want them, to help provide hints at the CLI level about 
which phase went wrong.

Yes, later. Don't worry about it right now. I am facing similar design 
consideration challenges for my Async QMP client over trying to decide 
which errors to "hide" or wrap and which to promote as interface. 
Ongoing learning process for me.

> I recommend to gloss over (irrelevant) details and say "For parse
> errors".  Yes, some of the errors aren't parse errors in the theory of
> parsing sense, but I doubt readers care.  If *you* care, then maybe "For
> errors in the schema source".  And then you might want to tweak the
> OSError explanation to "For problems reading the root schema source
> file".

I care a *little*. I am still trying to develop a sense of consistency 
for which things to document with :raise: and which I shouldn't.

(You are not the only person doing some guinea pig experiments and 
abusing a review process, you see ...)

I like the phrasing of "For errors in the schema source" more than "For 
parse errors" anyway. 1% less cryptic, even if the context is 
"inherently obvious".

> > +    """
> >       def __init__(self,
> >                    fname: str,
> >                    previously_included: Optional[Set[str]] = None,
> > @@ -73,6 +93,11 @@ def __init__(self,
> >           self._parse()
> >   
> >       def _parse(self) -> None:
> > +        """
> > +        Parse the QAPI schema document.
> > +
> > +        :return: None. Results are stored in ``.exprs`` and ``.docs``.
> > +        """
> >           cur_doc = None
> >   
> >           # May raise OSError; allow the caller to handle it.
> > @@ -199,6 +224,49 @@ def check_list_str(name: str, value: object) -> List[str]:
> >               raise QAPISemError(info, "unknown pragma '%s'" % name)
> >   
> >       def accept(self, skip_comment: bool = True) -> None:
> > +        """Read and store the next token.
> > +
> > +        :param skip_comment:
> > +            When false, return COMMENT tokens ("#").
> > +            This is used when reading documentation blocks.
> > +
> > +        :return:
> > +            None. Several instance attributes are updated instead:
> > +
> > +            - ``.tok`` represents the token type. See below for values.
> > +            - ``.info`` describes the token's source location.
> > +            - ``.val`` is the token's value, if any. See below.
> > +            - ``.pos`` is the buffer index of the first character of
> > +              the token.
> > +
> > +        * Single-character tokens:
> > +
> > +            These are "{", "}", ":", ",", "[", and "]". ``.tok`` holds
> > +            the single character and ``.val`` is None.
> > +
> > +        * Multi-character tokens:
> > +
> > +          * COMMENT:
> > +
> > +            This token is not normally returned by the lexer, but it can
> > +            be when ``skip_comment`` is False. ``.tok`` is "#", and
> > +            ``.val`` is a string including all chars until end-of-line,
> > +            including the "#" itself.
> > +
> > +          * STRING:
> > +
> > +            ``.tok`` is "'", the single quote. ``.val`` contains the
> > +            string, excluding the surrounding quotes.
> > +
> > +          * TRUE and FALSE:
> > +
> > +            ``.tok`` is either "t" or "f", ``.val`` will be the
> > +            corresponding bool value.
> > +
> > +          * EOF:
> > +
> > +            ``.tok`` and ``.val`` will both be None at EOF.
> > +        """
> >           while True:
> >               self.tok = self.src[self.cursor]
> >               self.pos = self.cursor
>
> This doc string is much better now, thanks!

Great! I took some liberties with your suggestions as I always do, but I 
like indicating the state changes in the :return: blurb in particular.

--js