John Snow <js...@redhat.com> writes:
> Here's the weird bit. QAPIDoc generally expects -- virtually everywhere
> -- that it will always have a current section. The sole exception to
> this is in the case that end_comment() is called, which leaves us with
> *no* section. However, in this case, we also don't expect to actually
> ever mutate the comment contents ever again.
>
> NullSection is just a Null-object that allows us to maintain the
> invariant that we *always* have a current section, enforced by static
> typing -- allowing us to type that field as QAPIDoc.Section instead of
> the more ambiguous Optional[QAPIDoc.Section].
>
> end_section is renamed to switch_section and now accepts as an argument
> the new section to activate, clarifying that no callers ever just
> unilaterally end a section; they only do so when starting a new section.
>
> Signed-off-by: John Snow <js...@redhat.com>
>
> ---
>
> For my money: Optional types can be a nuisance because an unfamiliar
> reader may wonder in what circumstances the field may be unset. This
> makes the condition quite a bit more explicit and statically provable.
>
> Doing it in this way (and not by creating a dummy section) will also
> continue to reject (rather noisily) any erroneous attempts to append
> additional lines after end_comment() has been called.
>
> Also, this section isn't indexed into .sections[] and isn't really
> visible in any way to external users of the class, so it seems like a
> harmless and low-cost way to formalize the "life cycle" of a QAPIDoc
> parser.
>
> Clean and clear as I can make it, in as few lines as I could muster.
>
> Signed-off-by: John Snow <js...@redhat.com>
> ---
> scripts/qapi/parser.py | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 1fdc5bc7056..123fc2f099c 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -478,6 +478,13 @@ def __init__(self, parser, name, indent=0):
> def connect(self, member):
> self.member = member
>
> + class NullSection(Section):
> + """
> + Empty section that signifies the end of a doc block.
What about "Dummy section for use at the end of a doc block"?
> + """
> + def append(self, line):
> + assert False, "BUG: Text appended after end_comment() called."
How can a failing assertion *not* be a bug?
> +
> 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.
> @@ -525,7 +532,7 @@ def append(self, line):
> self._append_line(line)
>
> def end_comment(self):
> - self._end_section()
> + self._switch_section(QAPIDoc.NullSection(self._parser))
>
> @staticmethod
> def _is_section_tag(name):
> @@ -702,9 +709,9 @@ def _start_symbol_section(self, symbols_dict, name,
> indent):
> raise QAPIParseError(self._parser,
> "'%s' parameter name duplicated" % name)
> assert not self.sections
> - self._end_section()
> - self._section = QAPIDoc.ArgSection(self._parser, name, indent)
> - symbols_dict[name] = self._section
> + new_section = QAPIDoc.ArgSection(self._parser, name, indent)
> + self._switch_section(new_section)
> + symbols_dict[name] = new_section
>
> def _start_args_section(self, name, indent):
> self._start_symbol_section(self.args, name, indent)
> @@ -716,13 +723,11 @@ def _start_section(self, name=None, indent=0):
> if name in ('Returns', 'Since') and self.has_section(name):
> raise QAPIParseError(self._parser,
> "duplicated '%s' section" % name)
> - self._end_section()
> - self._section = QAPIDoc.Section(self._parser, name, indent)
> - self.sections.append(self._section)
> -
> - def _end_section(self):
> - assert self._section is not None
> + new_section = QAPIDoc.Section(self._parser, name, indent)
> + self._switch_section(new_section)
> + self.sections.append(new_section)
>
> + def _switch_section(self, new_section):
> text = self._section.text = self._section.text.strip()
>
> # Only the 'body' section is allowed to have an empty body.
> @@ -735,7 +740,7 @@ def _end_section(self):
> self._parser,
> "empty doc section '%s'" % self._section.name)
>
> - self._section = None
> + self._section = new_section
>
> def _append_freeform(self, line):
> match = re.match(r'(@\S+:)', line)
Feels clearer, thanks!
