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

[Daniel P . Berrangé]

On Wed, Nov 22, 2023 at 10:50:47AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Nov 21, 2023 at 11:28:17AM -0500, John Snow wrote:
> >> 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
> >> 
> >> 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.
> >
> > The 'abc' module described is an official PEP standard
> >
> >   https://peps.python.org/pep-3119/
> 
> Compare:
> 
>     @abstractmethod
>     def c_type(self) -> str:
>         pass
> 
>     def c_type(self) -> str:
>         raise NotImplementedError()
> 
> I prefer the former, because it's more explicit.
> 
> Bonus: prevents accidental instantiation, and sub-classes don't need to
> know what's abstract in the super-class, they can blindly use super()
> calls. 

Being able to blindly call the parent impl via super() is more than
just a bonus, it is a super compelling reason to use this. It protects
your code against bugs from future re-factoring of the class hierarchy
whether adding or removing parents. Even if we don't expect to need it
for this particular class, I think this justifies having a policy of
using 'abc' everywhere we need abstract methods.

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 :|