| Message ID | 20231116014350.653792-6-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
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>
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.
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
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
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
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 = {
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(-)