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 = {
--
2.41.0John 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.
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 :|
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
>
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/
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 :|
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. docs.python.org:
Using this decorator requires that the class’s metaclass is ABCMeta
or is derived from it. A class that has a metaclass derived from
ABCMeta cannot be instantiated unless all of its abstract methods
and properties are overridden. The abstract methods can be called
using any of the normal ‘super’ call mechanisms. abstractmethod()
may be used to declare abstract methods for properties and
descriptors.
Hardly matters here, but since it's free...
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 :|
On 16/11/23 02:43, John Snow wrote: > 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(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2024 Red Hat, Inc.