On Wed, Mar 5, 2025, 1:28 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.com> writes:
>
> > Shhhhh!
> >
> > This patch is RFC quality, I wasn't in the mood to actually solve
> > problems so much as I was in the mood to continue working on the Sphinx
> > rework. Plus, I don't think the code I am patching has hit origin/master
> > yet ...
> >
> > Signed-off-by: John Snow <js...@redhat.com>
> > ---
> > scripts/qapi/backend.py | 2 ++
> > scripts/qapi/main.py | 10 ++++------
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/qapi/backend.py b/scripts/qapi/backend.py
> > index 14e60aa67af..49ae6ecdd33 100644
> > --- a/scripts/qapi/backend.py
> > +++ b/scripts/qapi/backend.py
> > @@ -13,6 +13,7 @@
> >
> >
> > class QAPIBackend(ABC):
> > + # pylint: disable=too-few-public-methods
> >
> > @abstractmethod
> > def generate(self,
> > @@ -36,6 +37,7 @@ def generate(self,
> >
> >
> > class QAPICBackend(QAPIBackend):
> > + # pylint: disable=too-few-public-methods
> >
> > def generate(self,
> > schema: QAPISchema,
>
> I didn't bother to ask for these in my review. Do they get us to the
> point where we can enable automatic pylint?
>
Yes.
> > diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> > index ff42b43cd68..01155373bd0 100644
> > --- a/scripts/qapi/main.py
> > +++ b/scripts/qapi/main.py
> > @@ -31,15 +31,14 @@ def create_backend(path: str) -> QAPIBackend:
> >
> > module_path, dot, class_name = path.rpartition('.')
> > if not dot:
> > - print(f"argument of -B must be of the form MODULE.CLASS",
> > + print("argument of -B must be of the form MODULE.CLASS",
>
> This one's already in "[PULL v2 0/5] QAPI patches patches for
> 2025-02-26". No worries.
>
Got it. I'll repull your tags.
> > file=sys.stderr)
> > sys.exit(1)
> >
> > try:
> > mod = import_module(module_path)
> > except Exception as ex:
> > - print(f"unable to import '{module_path}': {ex}",
> file=sys.stderr)
> > - sys.exit(1)
> > + raise QAPIError(f"unable to import '{module_path}': {ex}") from
> ex
> >
> > try:
> > klass = getattr(mod, class_name)
> > @@ -51,9 +50,8 @@ def create_backend(path: str) -> QAPIBackend:
> > try:
> > backend = klass()
> > except Exception as ex:
> > - print(f"backend '{path}' cannot be instantiated: {ex}",
> > - file=sys.stderr)
> > - sys.exit(1)
> > + raise QAPIError(
> > + f"backend '{path}' cannot be instantiated: {ex}") from ex
> >
> > if not isinstance(backend, QAPIBackend):
> > print(f"backend '{path}' must be an instance of QAPIBackend",
>
> Missed in my review: the caller catches QAPIError, and returns non-zero
> exit code on catch. The caller's caller passes the exit code to
> sys.exit(). Leaving the sys.exit() to the callers is cleaner.
>
> However, you convert only two out of five error paths from sys.exit() to
> raise. All or nothing, please.
>
"RFC Quality" ;)
You got it, I'll be consistent in approach here. My initial goal was only
to get the linters clean for this series.
> Maybe split the patch into a "# pylint:" and a "raise QAPIError" part?
>
Sure.
>
