| Message ID | 1489385927-6735-35-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Markus Armbruster <armbru@redhat.com> writes: > Results in a more precise error location, but the real reason is > emptying out check_docs() step by step. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Perhaps we should simply drop this error condition. Are empty sections this a mistake users make accidentally?
On 03/13/2017 01:18 AM, Markus Armbruster wrote: > Results in a more precise error location, but the real reason is > emptying out check_docs() step by step. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi.py | 20 ++++++++++++++------ > tests/qapi-schema/doc-empty-section.err | 2 +- > 2 files changed, 15 insertions(+), 7 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On 03/13/2017 01:23 AM, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> Results in a more precise error location, but the real reason is >> emptying out check_docs() step by step. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Perhaps we should simply drop this error condition. Are empty sections > this a mistake users make accidentally? Parse error; did you mean "empty sections _like_ this"? I'm okay with keeping the error; especially if we can't guarantee that the generator copes gracefully with an empty section (different than an omitted section). On the other hand, even if we remove the error, you're probably right that anyone proposing a patch for incorporation that adds an empty section will have to explain themselves, whether or not the parser flagged it, and if the error is cheap to maintain in the parser, then it saves some review cycles.
Eric Blake <eblake@redhat.com> writes: > On 03/13/2017 01:23 AM, Markus Armbruster wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Results in a more precise error location, but the real reason is >>> emptying out check_docs() step by step. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Perhaps we should simply drop this error condition. Are empty sections >> this a mistake users make accidentally? > > Parse error; did you mean "empty sections _like_ this"? Yes. > I'm okay with keeping the error; especially if we can't guarantee that > the generator copes gracefully with an empty section (different than an > omitted section). We'd have to verify it does. > On the other hand, even if we remove the error, > you're probably right that anyone proposing a patch for incorporation > that adds an empty section will have to explain themselves, whether or > not the parser flagged it, and if the error is cheap to maintain in the > parser, then it saves some review cycles. The patch adds two methods and changes three existing ones just to catch empty sections. I can't help to ask: why bother?
diff --git a/scripts/qapi.py b/scripts/qapi.py index 8d34651..e53701a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -173,6 +173,9 @@ class QAPIDoc(object): else: self._append_freeform(line) + def end_comment(self): + self._end_section() + def _append_symbol_line(self, line): name = line.split(' ', 1)[0] @@ -200,6 +203,7 @@ class QAPIDoc(object): raise QAPIParseError(self.parser, "'@%s:' can't follow '%s' section" % (name, self.sections[0].name)) + self._end_section() self.section = QAPIDoc.ArgSection(name) self.args[name] = self.section @@ -207,9 +211,18 @@ class QAPIDoc(object): 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(name) self.sections.append(self.section) + def _end_section(self): + if self.section: + contents = str(self.section) + if self.section.name and (not contents or contents.isspace()): + raise QAPIParseError(self.parser, "Empty doc section '%s'" + % self.section.name) + self.section = None + def _append_freeform(self, line): in_arg = isinstance(self.section, QAPIDoc.ArgSection) if (in_arg and self.section.content @@ -507,6 +520,7 @@ class QAPISchemaParser(object): if self.val != '##': raise QAPIParseError(self, "Junk after '##' at end of " "documentation comment") + doc.end_comment() self.accept() return doc else: @@ -1007,12 +1021,6 @@ def check_definition_doc(doc, expr, info): def check_docs(docs): for doc in docs: - for section in doc.args.values() + doc.sections: - content = str(section) - if not content or content.isspace(): - raise QAPISemError(doc.info, - "Empty doc section '%s'" % section.name) - if doc.expr: check_definition_doc(doc, doc.expr, doc.info) diff --git a/tests/qapi-schema/doc-empty-section.err b/tests/qapi-schema/doc-empty-section.err index 00ad625..b61e4a7 100644 --- a/tests/qapi-schema/doc-empty-section.err +++ b/tests/qapi-schema/doc-empty-section.err @@ -1 +1 @@ -tests/qapi-schema/doc-empty-section.json:3: Empty doc section 'Note' +tests/qapi-schema/doc-empty-section.json:7:1: Empty doc section 'Note'
Results in a more precise error location, but the real reason is emptying out check_docs() step by step. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi.py | 20 ++++++++++++++------ tests/qapi-schema/doc-empty-section.err | 2 +- 2 files changed, 15 insertions(+), 7 deletions(-)