Am 24.05.2019 um 18:11 hat Markus Armbruster geschrieben: > Kevin Wolf <kw...@redhat.com> 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 <kw...@redhat.com> > > --- > > 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