Michael Roth <mdr...@linux.vnet.ibm.com> writes: > Quoting Markus Armbruster (2018-02-11 03:35:52) >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> >> --- >> scripts/qapi/common.py | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >> index dce289ae21..cc5a5941dd 100644 >> --- a/scripts/qapi/common.py >> +++ b/scripts/qapi/common.py >> @@ -290,8 +290,12 @@ class QAPISchemaParser(object): >> if not isinstance(include, str): >> raise QAPISemError(info, >> "Value of 'include' must be a >> string") >> - self._include(include, info, os.path.dirname(self.fname), >> - previously_included) >> + exprs_include = self._include(include, info, >> + os.path.dirname(self.fname), >> + previously_included) >> + if exprs_include: >> + self.exprs.extend(exprs_include.exprs) >> + self.docs.extend(exprs_include.docs) >> elif "pragma" in expr: >> self.reject_expr_doc(cur_doc) >> if len(expr) != 1: >> @@ -334,14 +338,13 @@ class QAPISchemaParser(object): >> >> # skip multiple include of the same file >> if incl_abs_fname in previously_included: >> - return >> + return None >> + >> try: >> fobj = open(incl_fname, 'r') >> except IOError as e: >> raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname)) >> - exprs_include = QAPISchemaParser(fobj, previously_included, info) >> - self.exprs.extend(exprs_include.exprs) >> - self.docs.extend(exprs_include.docs) > > minor nit, but the function of _include() seems more appropriately > described now as _parse_include() or _parse_if_new() or something similar.
I'm open to renaming, but "parse" doesn't really fit. _include()'s caller checks syntax, then calls _include() to check semantic constraints and evaluate the include, then splices in the evaluated result. > Maybe it would be more readable to just move the previously_included > checks into init() and just drop _include() entirely? Maybe. But the conditional gets rather long then. > Reviewed-by: Michael Roth <mdr...@linux.vnet.ibm.com> Thanks!