Re: [PATCH 05/19] qapi/schema: make c_type() and json_type() abstract methods

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2843 bytes --]

On Tue, Nov 21, 2023, 8:43 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Nov 21, 2023 at 02:36:54PM +0100, Markus Armbruster wrote:
> > John Snow <jsnow@redhat.com> writes:
> >
> > > These methods should always return a str, it's only the default
> abstract
> > > implementation that doesn't. They can be marked "abstract" by raising
> > > NotImplementedError(), which requires subclasses to override the method
> > > with the proper return type.
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >  scripts/qapi/schema.py | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > > index c5fdd625452..4600a566005 100644
> > > --- a/scripts/qapi/schema.py
> > > +++ b/scripts/qapi/schema.py
> > > @@ -233,8 +233,8 @@ def visit(self, visitor):
> > >  class QAPISchemaType(QAPISchemaEntity):
> > >      # Return the C type for common use.
> > >      # For the types we commonly box, this is a pointer type.
> > > -    def c_type(self):
> > > -        pass
> > > +    def c_type(self) -> str:
> > > +        raise NotImplementedError()
> > >
> > >      # Return the C type to be used in a parameter list.
> > >      def c_param_type(self):
> > > @@ -244,8 +244,8 @@ def c_param_type(self):
> > >      def c_unboxed_type(self):
> > >          return self.c_type()
> > >
> > > -    def json_type(self):
> > > -        pass
> > > +    def json_type(self) -> str:
> > > +        raise NotImplementedError()
> > >
> > >      def alternate_qtype(self):
> > >          json2qtype = {
> >
> > I wish abstract methods could be done in a more concise way.
>
> The canonical way would be to use the 'abc' module:
>
>   from abc import ABCMeta, abstractmethod
>
>   class A(metaclass=ABCMeta):
>       @abstractmethod
>       def foo(self): pass
>
> Not sure if the @abstractmethod decorator is enough to keep the static
> typing checker happy about a 'str' return type, when there is no body
> impl
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>

In practice, I'm under the belief that mypy and pylint both recognize a
method raising NotImplementedError as marking an abstract method without
needing to explicitly inherit from the ABC.

I suppose there's also
https://docs.python.org/3/library/abc.html#abc.abstractmethod which I am
guessing just does this same thing. I'll see what makes mypy happy. I'm
assuming Markus would like to see something like this decorator to make it
more obvious that it's an abstract method.

--js

>

[-- Attachment #2: Type: text/html, Size: 4668 bytes --]