| Message ID | 20180211093607.27351-15-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/11/2018 03:35 AM, Markus Armbruster wrote: > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > scripts/qapi/common.py | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) Again, not much mention of 'why' in the commit message. But centralizing the initialization operations in one place rather than having to chase down multiple places is good. Reviewed-by: Eric Blake <eblake@redhat.com>
Quoting Markus Armbruster (2018-02-11 03:35:52) > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@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. Maybe it would be more readable to just move the previously_included checks into init() and just drop _include() entirely? Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > + return QAPISchemaParser(fobj, previously_included, info) > > def _pragma(self, name, value, info): > global doc_required, returns_whitelist, name_case_whitelist > -- > 2.13.6 >
Michael Roth <mdroth@linux.vnet.ibm.com> writes: > Quoting Markus Armbruster (2018-02-11 03:35:52) >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Reviewed-by: Marc-André Lureau <marcandre.lureau@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 <mdroth@linux.vnet.ibm.com> Thanks!
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) + return QAPISchemaParser(fobj, previously_included, info) def _pragma(self, name, value, info): global doc_required, returns_whitelist, name_case_whitelist