John Snow <jsnow@redhat.com> writes:
> It simplifies the typing to say that _section is always a
> QAPIDoc.Section().
If you say so....
I mean, I thought so at the time. I have an aversion to making optional types and then littering isinstance() checks in places. I like keeping as much of the typing statically provable as I can without adding runtime checks. I'm re-evaluating this patch now, and I'll see if there's anything I can do, but it's hard to predict differences in matters of taste and style.
One thing an Optional[T] class instance variable can do that might be helpful is to remind users that they need to check to see if it's present or not. If I see a path to eliminate those checks, though, I will generally always take it - eliminates the need to check everywhere else and puts all the information you need into the static typing. As a habit, I prefer that when feasible.
> To accommodate this change, we must allow for this object to evaluate to
> False for functions like _end_section which behave differently based on
> whether or not a Section has been started.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> Probably a better fix is to restructure the code to prevent empty
> sections from being "ended", but that seems like a bigger whale than
> what I'm after at the immediate moment.
Do we have a TODO comment for that?
Nope. I'll either add one or just fix the root issue, because I want to go back to writing the cross-referenceable QAPI symbol plugin for sphinx now. At the time I thought I'd get to fixing some of the issues raised by pt5 a lot sooner, but.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/parser.py | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b6a5e661215..3ddde318376 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -456,6 +456,9 @@ def __init__(self, parser, name=None, indent=0):
> # the expected indent level of the text of this section
> self._indent = indent
>
> + def __bool__(self) -> bool:
> + return bool(self.name or self.text)
> +
> def append(self, line):
> # Strip leading spaces corresponding to the expected indent level
> # Blank lines are always OK.
Overriding __bool__() is the minimally invasive compensation for the
next hunk's replacement of None by a QAPIDoc.Section
However, I'm wary of overriding __bool__(). It creates a difference
between "if object:" and "if object is not None:". Gives me a queasy
feeling, as shortening the latter to the former is pretty much
automatic.
That's just Python, though. If collections are empty, they're falsey. If QAPIDoc is a collection of documentation lines and we have no lines, shouldn't that also be falsey?
Nah, I get the apprehension. It's not a strictly linear collection and I didn't check *every* last field, just name and text. It's kind of a hack because I'm trying to paper over some deeper(?) problem(??), but it felt safe because the static typing means we're not likely to confuse the two cases -- we don't need to distinguish "Absent" from "empty". Or at least, after this patch we don't need to, anymore. (We'll always have a section, so an empty section versus no section makes no difference.)
A boring .is_empty() would avoid that, but we'd have to adjust "if S" to
"if S.is_empty()" wherever we changed S from Optional[Section] to
Section. Which S could be affected?
Feels deeply against the grain of how Python is written to go out of your way to create an .is_empty() function.
The following variables get assigned Section or ArgSection:
QAPIDoc.body
QAPIDoc._section
QAPIDoc.args[NAME]
QAPIDoc.features[NAME]
.body, .args[NAME] and .features[NAME] are never None I believe.
._section is also assigned None, in ._end_section(). It remains None
until the next ._start*_section().
The only use of .section that doesn't dot into it is in ._end_section().
That's the only spot to adjust.
Confirm by testing: in all of "make check", Section.__bool__() is only
ever called from QAPIDoc._end_section(). Checked by sticking
traceback.print_stack() into .__bool__().
If that's the only caller, isn't it then perfectly safe to just use the patch as written?
> @@ -722,7 +725,7 @@ def _end_section(self):
> raise QAPIParseError(
> self._parser,
> "empty doc section '%s'" % self._section.name)
> - self._section = None
> + self._section = QAPIDoc.Section(self._parser)
>
> def _append_freeform(self, line):
> match = re.match(r'(@\S+:)', line)
Replacing None by QAPIDoc.Section doesn't just simplify the typing! It
also creates a bogus "additional section" (in the sense of QAPIDoc's
class comment) after any section. Works, because the .start*_section()
all call .end_section() first, .end_section() does nothing for empty
sections, and the bogus sections remain empty, unless we somehow screw
up the code to add contents to them. Such screwups are now possible,
whereas before they would crash.
This correctness argument isn't obvious, and therefore should be made in
the commit message.
I'll try to suss out the real problem, because I am not sure how to justify the hypothetical cases where we might add content to a bogus section, because I do not fully understand the circumstances in which the bogus sections are generated. I seem to recall we already *have* them for some reason, and it's caused by some whitespace issues (in some cases), but writing tests or proving it doesn't cause a hypothetical future breakage is just not something I know how to write a commit message for.
So, time to detour into QAPIDoc parsing.
--js