John Snow <js...@redhat.com> writes:
> On 5/21/21 10:48 AM, Markus Armbruster wrote:
>> Beware, I'm skimming, not really reviewing.
>>
>> John Snow <js...@redhat.com> writes:
>>
>>> On 4/29/21 9:40 AM, marcandre.lur...@redhat.com wrote:
[...]
>>>> index b7f475a160..59a7ee2f32 100644
>>>> --- a/scripts/qapi/common.py
>>>> +++ b/scripts/qapi/common.py
>>>> @@ -11,8 +11,9 @@
>>>> # This work is licensed under the terms of the GNU GPL, version 2.
>>>> # See the COPYING file in the top-level directory.
>>>> +from abc import ABC, abstractmethod
>>>> import re
>>>> -from typing import Optional
>>>> +from typing import Optional, Sequence
>>>>
>>>> #: Magic string that gets removed along with all space to its right.
>>>> @@ -192,3 +193,54 @@ def guardend(name: str) -> str:
>>>> #endif /* %(name)s */
>>>> ''',
>>>> name=c_fname(name).upper())
>>>> +
>>>> +
>>>> +class IfPredicate(ABC):
>>>> + @abstractmethod
>>>> + def cgen(self) -> str:
>>>
>>> Like the review to patch 2, I'm not sure we want to bake cgen stuff
>>> directly into this class. Are you going to have cgen() and rustgen()
>>> methods all here?
>>>
>>>> + pass
>>>> +
>>>
>>> I think you want raise NotImplementedError to specify a function that
>>> the inheriting class MUST implement. Otherwise, there's little value
>>> to allow a child class to call super() on a method that doesn't have a
>>> default implementation.
>>>
>>> You *could* drop the (ABC) and the @abstractmethod decorators if you do so.
>>>
>>> Matters of taste and style.
>>
>> We're not coding a library for use by thousands of people. If we did,
>> then complicating the code to guard against misuse could be a win. But
>> we don't.
>>
>> schema.py is full of methods that pass. Maybe some of them need to be
>> overriden by any conceivable subtype. Who cares? The subtypes either
>> work or they don't. I prefer
>>
>> def frobnicate:
>> pass
>>
>> to
>>
>> def frobnicate:
>> raise NotImplementedError
>>
>> One, pass is easier on the eyes. Two, a subtype's .frobnicate() can
>> blindly call super().frobnicate().
>>
>
> "pass" here operates as a syntactic sugar for "return None" which has
> implications on the return type. It's short, but wrong.
Nitpicking...
pass is not specified as syntactic sugar:
7.4. The pass statement
pass_stmt ::= "pass"
pass is a null operation — when it is executed, nothing happens. It is
useful as a placeholder when a statement is required syntactically, but no code
needs to be executed, for example:
def f(arg): pass # a function that does nothing (yet)
class C: pass # a class with no methods (yet)
What really happens in
def foo(): pass
is what *always* happens when control reaches the end of the function
body: the function returns None.
Further evidence:
>>> def baz():
... pass
... return 42
...
>>> baz()
42
> raise NotImplementedError means something different semantically.
>
> To me, pass is *NOT* easier on the eyes, it is misleading. It is
> idiomatic to use NotImplementedError if it is not acceptable for a
> default implementation to return None.
>
> I understand perfectly well the desire to keep things simple, but what
> is actually "simple" depends on the expectations of the programmer. I
> err towards idiomatic Python.
What's your take on "Two, a subtype's .frobnicate() can blindly call
super().frobnicate()"?
[...]
