[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code |
Date: |
Thu, 30 May 2019 00:09:15 +0200 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
Am 24.05.2019 um 18:11 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
>
> > Documentation comment follow a certain structure: First, we have a text
> > with a general description (called QAPIDoc.body). After this,
> > descriptions of the arguments follow. Finally, we have part that
> > contains various named sections.
> >
> > The code doesn't show this structure but just checks the right side
> > conditions so it happens to do the right set of things in the right
>
> What are "side conditions"?
>
> > phase. This is hard to follow, and adding support for documentation of
> > features would be even harder.
> >
> > This restructures the code so that the three parts are clearly
> > separated. The code becomes a bit longer, but easier to follow.
>
> Recommend to mention that output remains unchanged.
>
> >
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> > scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
> > 1 file changed, 83 insertions(+), 24 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 71944e2e30..1d0f4847db 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -120,6 +120,27 @@ class QAPIDoc(object):
> > def connect(self, member):
> > self.member = member
> >
> > + class SymbolPart:
> > + """
> > + Describes which part of the documentation we're parsing right now.
>
> So SymbolPart is a part of the documentation. Shouldn't it be named
> DocPart then?
That's a better name. I was stuck in the old code (which was concerned
about what a symbol name means at which point) rather than thinking
about high-level concepts.
> > +
> > + BODY means that we're ready to process freeform text into
> > self.body. A
>
> s/freeform/free-form/
Both are valid spellings and I generally don't expect correct spellings
to be corrected, but arguably "free-form" is more standard. I'll change
it. (If we were consistent, the method should have been named
_append_free_form rather than _append_freeform originally...)
> > + symbol name is only allowed if no other text was parsed yet. It is
>
> Start your sentences with a capital letter.
I would gladly correct a sentence not starting with a capital letter if
I could see any. The quoted sentence starts with a capital "A" in the
previous line.
> > + interpreted as the symbol name that describes the currently
> > documented
> > + object. On getting the second symbol name, we proceed to ARGS.
> > +
> > + ARGS means that we're parsing the arguments section. Any symbol
> > name is
> > + interpreted as an argument and an ArgSection is created for it.
> > +
> > + VARIOUS is the final part where freeform sections may appear. This
> > + includes named sections such as "Return:" as well as unnamed
> > + paragraphs. No symbols are allowed any more in this part.
>
> s/any more/anymore/
Again both are valid, but this time, "any more" is the more standard
spelling.
> > + # Can't make it a subclass of Enum because of Python 2
>
> Thanks for documenting Python 2 contortions! Let's phrase it as a TODO
> comment.
>
> > + BODY = 0
>
> Any particular reason for 0?
>
> As far as I can tell, Python enum values commonly start with 1, to make
> them all true.
If you use enums in a boolean context, you're doing something wrong
anyway. *shrug*
I'll change it, it's consistent with real Enum classes where the values
becomes non-integer objects (which therefore evaluate as True in boolean
contexts).
> > + ARGS = 1
> > + VARIOUS = 2
> > +
> > def __init__(self, parser, info):
> > # self._parser is used to report errors with QAPIParseError. The
> > # resulting error position depends on the state of the parser.
> > @@ -135,6 +156,7 @@ class QAPIDoc(object):
> > self.sections = []
> > # the current section
> > self._section = self.body
> > + self._part = QAPIDoc.SymbolPart.BODY
>
> The right hand side is tiresome, but I don't have better ideas.
This is just what Python enums look like... I could move the class
outside of QAPIDoc to save that part of the prefix, but I'd prefer not
to.
> >
> > def has_section(self, name):
> > """Return True if we have a section with this name."""
> > @@ -154,37 +176,84 @@ class QAPIDoc(object):
> def append(self, line):
> """Parse a comment line and add it to the documentation."""
> line = line[1:]
> if not line:
> self._append_freeform(line)
> return
>
> if line[0] != ' ':
> > raise QAPIParseError(self._parser, "Missing space after #")
> > line = line[1:]
> >
> > + if self._part == QAPIDoc.SymbolPart.BODY:
> > + self._append_body_line(line)
> > + elif self._part == QAPIDoc.SymbolPart.ARGS:
> > + self._append_args_line(line)
> > + elif self._part == QAPIDoc.SymbolPart.VARIOUS:
> > + self._append_various_line(line)
> > + else:
> > + assert False
>
> Hmm. As far as I can tell, this what we use ._part for. All other
> occurences assign to it.
>
> If you replace
>
> self._part = QAPIDoc.SymbolPart.BODY
>
> by
>
> self._append_line = self._append_body_line
>
> and so forth, then the whole conditional shrinks to
>
> self._append_line(line)
>
> and we don't have to muck around with enums.
I could just have added a boolean that decides whether a symbol is an
argument or a feature. That would have been a minimal hack that
wouldn't involve any enums.
I intentionally decided not to do that because the whole structure of
the parser was horribly confusing to me and I felt that introducing a
clear state machine would improve its legibility a lot. I still think
that this is what it did.
If you don't like a proper state machine, I can do that bool thing. I
don't think throwing in function pointers would be very helpful for
readers, so we'd get a major code change for no gain.
Kevin
- [Qemu-devel] [PATCH v2 0/6] file-posix: Add dynamic-auto-read-only QAPI feature, Kevin Wolf, 2019/05/17
- [Qemu-devel] [PATCH v2 1/6] qapi: Support features for structs, Kevin Wolf, 2019/05/17
- [Qemu-devel] [PATCH v2 2/6] tests/qapi-schema: Test for good feature lists in structs, Kevin Wolf, 2019/05/17
- [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code, Kevin Wolf, 2019/05/17
- [Qemu-devel] [PATCH v2 5/6] qapi: Allow documentation for features, Kevin Wolf, 2019/05/17
- [Qemu-devel] [PATCH v2 3/6] tests/qapi-schema: Error case tests for features in structs, Kevin Wolf, 2019/05/17
- [Qemu-devel] [PATCH v2 6/6] file-posix: Add dynamic-auto-read-only QAPI feature, Kevin Wolf, 2019/05/17
- Re: [Qemu-devel] [PATCH v2 0/6] file-posix: Add dynamic-auto-read-only QAPI feature, Markus Armbruster, 2019/05/24