diff mbox series

[v2,06/11] qapi/introspect.py: add _gen_features helper

Message ID 20201026194251.11075-7-jsnow@redhat.com
State Superseded
Headers show
Series [v2,01/11,DO-NOT-MERGE] docs: replace single backtick (`) with double-backtick (``) | expand

Commit Message

John Snow Oct. 26, 2020, 7:42 p.m. UTC
_make_tree might receive a dict or some other type. Adding features
information should arguably be performed by the caller at such a time
when we know the type of the object and don't have to re-interrogate it.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/introspect.py | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Cleber Rosa Nov. 7, 2020, 4:23 a.m. UTC | #1
On Mon, Oct 26, 2020 at 03:42:46PM -0400, John Snow wrote:
> _make_tree might receive a dict or some other type. Adding features

> information should arguably be performed by the caller at such a time

> when we know the type of the object and don't have to re-interrogate it.

> 

> Signed-off-by: John Snow <jsnow@redhat.com>

> ---


Reviewed-by: Cleber Rosa <crosa@redhat.com>
Markus Armbruster Nov. 16, 2020, 8:47 a.m. UTC | #2
John Snow <jsnow@redhat.com> writes:

> _make_tree might receive a dict or some other type.


Are you talking about @obj?

>                                                     Adding features

> information should arguably be performed by the caller at such a time

> when we know the type of the object and don't have to re-interrogate it.


Fair enough.  There are just two such callers anyway.

> Signed-off-by: John Snow <jsnow@redhat.com>

> ---

>  scripts/qapi/introspect.py | 19 ++++++++++++-------

>  1 file changed, 12 insertions(+), 7 deletions(-)

>

> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py

> index 803288a64e7..16282f2634b 100644

> --- a/scripts/qapi/introspect.py

> +++ b/scripts/qapi/introspect.py

> @@ -76,16 +76,12 @@

>  

>  

>  def _make_tree(obj: Union[_DObject, str], ifcond: List[str],

> -               features: List[QAPISchemaFeature],

>                 extra: Optional[Annotations] = None

>                 ) -> TreeValue:

>      if extra is None:

>          extra = {}

>      if ifcond:

>          extra['if'] = ifcond

> -    if features:

> -        assert isinstance(obj, dict)

> -        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]

>      if extra:

>          return (obj, extra)

>      return obj

> @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:

>              return '[' + self._use_type(typ.element_type) + ']'

>          return self._name(typ.name)

>  

> +    @classmethod

> +    def _gen_features(cls,

> +                      features: List[QAPISchemaFeature]) -> List[TreeValue]:

> +        return [_make_tree(f.name, f.ifcond) for f in features]

> +


Ignorant question: when to use @classmethod, and when to use
@staticmethod?

>      def _gen_tree(self, name: str, mtype: str, obj: _DObject,

>                    ifcond: List[str],

>                    features: Optional[List[QAPISchemaFeature]]) -> None:

> @@ -233,7 +234,9 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,

>              name = self._name(name)

>          obj['name'] = name

>          obj['meta-type'] = mtype

> -        self._trees.append(_make_tree(obj, ifcond, features, extra))

> +        if features:

> +            obj['features'] = self._gen_features(features)

> +        self._trees.append(_make_tree(obj, ifcond, extra))

>  

>      def _gen_member(self,

>                      member: QAPISchemaObjectTypeMember) -> TreeValue:


No change when not features.  Else, you change

    obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]

to

    obj['features'] = [_make_tree(f.name, f.ifcond) for f in features]

where

    _make_tree(f.name, f.ifcond)
    = (f.name, {'if': f.ifcond})       if f.ifcond
    = f.name                           else

Works, and feels less lazy.  However, the commit message did not prepare
me for this.  If you split this off into its own patch, you can describe
it properly.

> @@ -243,7 +246,9 @@ def _gen_member(self,

>          }

>          if member.optional:

>              obj['default'] = None

> -        return _make_tree(obj, member.ifcond, member.features)

> +        if member.features:

> +            obj['features'] = self._gen_features(member.features)

> +        return _make_tree(obj, member.ifcond)

>  

>      def _gen_variants(self, tag_name: str,

>                        variants: List[QAPISchemaVariant]) -> _DObject:

> @@ -255,7 +260,7 @@ def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:

>              'case': variant.name,

>              'type': self._use_type(variant.type)

>          }

> -        return _make_tree(obj, variant.ifcond, None)

> +        return _make_tree(obj, variant.ifcond)

>  

>      def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],

>                             json_type: str) -> None:
John Snow Dec. 7, 2020, 11:57 p.m. UTC | #3
On 11/16/20 3:47 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:

> 

>> _make_tree might receive a dict or some other type.

> 

> Are you talking about @obj?

> 


Yes. It *usually* takes a dict. sometimes it doesn't.

>>                                                      Adding features

>> information should arguably be performed by the caller at such a time

>> when we know the type of the object and don't have to re-interrogate it.

> 

> Fair enough.  There are just two such callers anyway.

> 

>> Signed-off-by: John Snow <jsnow@redhat.com>

>> ---

>>   scripts/qapi/introspect.py | 19 ++++++++++++-------

>>   1 file changed, 12 insertions(+), 7 deletions(-)

>>

>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py

>> index 803288a64e7..16282f2634b 100644

>> --- a/scripts/qapi/introspect.py

>> +++ b/scripts/qapi/introspect.py

>> @@ -76,16 +76,12 @@

>>   

>>   

>>   def _make_tree(obj: Union[_DObject, str], ifcond: List[str],

>> -               features: List[QAPISchemaFeature],

>>                  extra: Optional[Annotations] = None

>>                  ) -> TreeValue:

>>       if extra is None:

>>           extra = {}

>>       if ifcond:

>>           extra['if'] = ifcond

>> -    if features:

>> -        assert isinstance(obj, dict)

>> -        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]

>>       if extra:

>>           return (obj, extra)

>>       return obj

>> @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:

>>               return '[' + self._use_type(typ.element_type) + ']'

>>           return self._name(typ.name)

>>   

>> +    @classmethod

>> +    def _gen_features(cls,

>> +                      features: List[QAPISchemaFeature]) -> List[TreeValue]:

>> +        return [_make_tree(f.name, f.ifcond) for f in features]

>> +

> 

> Ignorant question: when to use @classmethod, and when to use

> @staticmethod?

> 


Matter of taste. My preference is to just always use @classmethod, 
because they can be extended or referenced by subclasses.

@staticmethod does not take a class argument, @classmethod does. Static 
methods therefore cannot address any other classmethods, but a 
classmethod can.

I just always reach for classmethod by default.

>>       def _gen_tree(self, name: str, mtype: str, obj: _DObject,

>>                     ifcond: List[str],

>>                     features: Optional[List[QAPISchemaFeature]]) -> None:

>> @@ -233,7 +234,9 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,

>>               name = self._name(name)

>>           obj['name'] = name

>>           obj['meta-type'] = mtype

>> -        self._trees.append(_make_tree(obj, ifcond, features, extra))

>> +        if features:

>> +            obj['features'] = self._gen_features(features)

>> +        self._trees.append(_make_tree(obj, ifcond, extra))

>>   

>>       def _gen_member(self,

>>                       member: QAPISchemaObjectTypeMember) -> TreeValue:

> 

> No change when not features.  Else, you change

> 

>      obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]

> 

> to

> 

>      obj['features'] = [_make_tree(f.name, f.ifcond) for f in features]

> 


Yep. I am consolidating the logic for (node, annotation) in so doing.

> where

> 

>      _make_tree(f.name, f.ifcond)

>      = (f.name, {'if': f.ifcond})       if f.ifcond

>      = f.name                           else

> 

> Works, and feels less lazy.  However, the commit message did not prepare

> me for this.  If you split this off into its own patch, you can describe

> it properly.

> 


OK.

>> @@ -243,7 +246,9 @@ def _gen_member(self,

>>           }

>>           if member.optional:

>>               obj['default'] = None

>> -        return _make_tree(obj, member.ifcond, member.features)

>> +        if member.features:

>> +            obj['features'] = self._gen_features(member.features)

>> +        return _make_tree(obj, member.ifcond)

>>   

>>       def _gen_variants(self, tag_name: str,

>>                         variants: List[QAPISchemaVariant]) -> _DObject:

>> @@ -255,7 +260,7 @@ def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:

>>               'case': variant.name,

>>               'type': self._use_type(variant.type)

>>           }

>> -        return _make_tree(obj, variant.ifcond, None)

>> +        return _make_tree(obj, variant.ifcond)

>>   

>>       def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],

>>                              json_type: str) -> None:
Markus Armbruster Dec. 15, 2020, 4:55 p.m. UTC | #4
John Snow <jsnow@redhat.com> writes:

> On 11/16/20 3:47 AM, Markus Armbruster wrote:

>> John Snow <jsnow@redhat.com> writes:

>> 

>>> _make_tree might receive a dict or some other type.

>> 

>> Are you talking about @obj?

>> 

>

> Yes.


Recommend to be explict: _make_tree()'s first argument can be ...

>      It *usually* takes a dict. sometimes it doesn't.


Yes.  It takes an abstract syntax tree: dict for JSON object, list for
JSON array, str for JSON string, bool for JSON true and false, NoneType
for JSON none.  JSON int isn't implemented, because it doesn't occur in
SchemaInfo.

>>>                                                      Adding features

>>> information should arguably be performed by the caller at such a time

>>> when we know the type of the object and don't have to re-interrogate it.

>> 

>> Fair enough.  There are just two such callers anyway.

>> 

>>> Signed-off-by: John Snow <jsnow@redhat.com>

>>> ---

>>>   scripts/qapi/introspect.py | 19 ++++++++++++-------

>>>   1 file changed, 12 insertions(+), 7 deletions(-)

>>>

>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py

>>> index 803288a64e7..16282f2634b 100644

>>> --- a/scripts/qapi/introspect.py

>>> +++ b/scripts/qapi/introspect.py

>>> @@ -76,16 +76,12 @@

>>>   

>>>   

>>>   def _make_tree(obj: Union[_DObject, str], ifcond: List[str],

>>> -               features: List[QAPISchemaFeature],

>>>                  extra: Optional[Annotations] = None

>>>                  ) -> TreeValue:

>>>       if extra is None:

>>>           extra = {}

>>>       if ifcond:

>>>           extra['if'] = ifcond

>>> -    if features:

>>> -        assert isinstance(obj, dict)

>>> -        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]

>>>       if extra:

>>>           return (obj, extra)

>>>       return obj

>>> @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:

>>>               return '[' + self._use_type(typ.element_type) + ']'

>>>           return self._name(typ.name)

>>>   

>>> +    @classmethod

>>> +    def _gen_features(cls,

>>> +                      features: List[QAPISchemaFeature]) -> List[TreeValue]:

>>> +        return [_make_tree(f.name, f.ifcond) for f in features]

>>> +

>> 

>> Ignorant question: when to use @classmethod, and when to use

>> @staticmethod?

>

> Matter of taste. My preference is to just always use @classmethod, 

> because they can be extended or referenced by subclasses.


Non-issue here, sub-classes are vanishingly unlikely.

> @staticmethod does not take a class argument, @classmethod does. Static 

> methods therefore cannot address any other classmethods, but a 

> classmethod can.

>

> I just always reach for classmethod by default.


Unused cls parameters are slightly annoying, though.

I've been using @staticmethod whenever it suffices.  Makes "this is a
function, i.e. it can't mess with the class or instances" immediately
obvious.

[...]
John Snow Dec. 15, 2020, 6:49 p.m. UTC | #5
On 12/15/20 11:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:

> 

>> On 11/16/20 3:47 AM, Markus Armbruster wrote:

>>> John Snow <jsnow@redhat.com> writes:

>>>

>>>> _make_tree might receive a dict or some other type.

>>>

>>> Are you talking about @obj?

>>>

>>

>> Yes.

> 

> Recommend to be explict: _make_tree()'s first argument can be ...

> 

>>       It *usually* takes a dict. sometimes it doesn't.

> 

> Yes.  It takes an abstract syntax tree: dict for JSON object, list for

> JSON array, str for JSON string, bool for JSON true and false, NoneType

> for JSON none.  JSON int isn't implemented, because it doesn't occur in

> SchemaInfo.

> 

>>>>                                                       Adding features

>>>> information should arguably be performed by the caller at such a time

>>>> when we know the type of the object and don't have to re-interrogate it.

>>>

>>> Fair enough.  There are just two such callers anyway.

>>>

>>>> Signed-off-by: John Snow <jsnow@redhat.com>

>>>> ---

>>>>    scripts/qapi/introspect.py | 19 ++++++++++++-------

>>>>    1 file changed, 12 insertions(+), 7 deletions(-)

>>>>

>>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py

>>>> index 803288a64e7..16282f2634b 100644

>>>> --- a/scripts/qapi/introspect.py

>>>> +++ b/scripts/qapi/introspect.py

>>>> @@ -76,16 +76,12 @@

>>>>    

>>>>    

>>>>    def _make_tree(obj: Union[_DObject, str], ifcond: List[str],

>>>> -               features: List[QAPISchemaFeature],

>>>>                   extra: Optional[Annotations] = None

>>>>                   ) -> TreeValue:

>>>>        if extra is None:

>>>>            extra = {}

>>>>        if ifcond:

>>>>            extra['if'] = ifcond

>>>> -    if features:

>>>> -        assert isinstance(obj, dict)

>>>> -        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]

>>>>        if extra:

>>>>            return (obj, extra)

>>>>        return obj

>>>> @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:

>>>>                return '[' + self._use_type(typ.element_type) + ']'

>>>>            return self._name(typ.name)

>>>>    

>>>> +    @classmethod

>>>> +    def _gen_features(cls,

>>>> +                      features: List[QAPISchemaFeature]) -> List[TreeValue]:

>>>> +        return [_make_tree(f.name, f.ifcond) for f in features]

>>>> +

>>>

>>> Ignorant question: when to use @classmethod, and when to use

>>> @staticmethod?

>>

>> Matter of taste. My preference is to just always use @classmethod,

>> because they can be extended or referenced by subclasses.

> 

> Non-issue here, sub-classes are vanishingly unlikely.

> 


True. I did admit it was just simply my default. I can adjust it 
case-by-case for circumstances when ...

>> @staticmethod does not take a class argument, @classmethod does. Static

>> methods therefore cannot address any other classmethods, but a

>> classmethod can.

>>

>> I just always reach for classmethod by default.

> 

> Unused cls parameters are slightly annoying, though.

> 

> I've been using @staticmethod whenever it suffices.  Makes "this is a

> function, i.e. it can't mess with the class or instances" immediately

> obvious.

> 


... you feel it provides additional clarity to do so.

--js
diff mbox series

Patch

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 803288a64e7..16282f2634b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -76,16 +76,12 @@ 
 
 
 def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
-               features: List[QAPISchemaFeature],
                extra: Optional[Annotations] = None
                ) -> TreeValue:
     if extra is None:
         extra = {}
     if ifcond:
         extra['if'] = ifcond
-    if features:
-        assert isinstance(obj, dict)
-        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
     if extra:
         return (obj, extra)
     return obj
@@ -221,6 +217,11 @@  def _use_type(self, typ: QAPISchemaType) -> str:
             return '[' + self._use_type(typ.element_type) + ']'
         return self._name(typ.name)
 
+    @classmethod
+    def _gen_features(cls,
+                      features: List[QAPISchemaFeature]) -> List[TreeValue]:
+        return [_make_tree(f.name, f.ifcond) for f in features]
+
     def _gen_tree(self, name: str, mtype: str, obj: _DObject,
                   ifcond: List[str],
                   features: Optional[List[QAPISchemaFeature]]) -> None:
@@ -233,7 +234,9 @@  def _gen_tree(self, name: str, mtype: str, obj: _DObject,
             name = self._name(name)
         obj['name'] = name
         obj['meta-type'] = mtype
-        self._trees.append(_make_tree(obj, ifcond, features, extra))
+        if features:
+            obj['features'] = self._gen_features(features)
+        self._trees.append(_make_tree(obj, ifcond, extra))
 
     def _gen_member(self,
                     member: QAPISchemaObjectTypeMember) -> TreeValue:
@@ -243,7 +246,9 @@  def _gen_member(self,
         }
         if member.optional:
             obj['default'] = None
-        return _make_tree(obj, member.ifcond, member.features)
+        if member.features:
+            obj['features'] = self._gen_features(member.features)
+        return _make_tree(obj, member.ifcond)
 
     def _gen_variants(self, tag_name: str,
                       variants: List[QAPISchemaVariant]) -> _DObject:
@@ -255,7 +260,7 @@  def _gen_variant(self, variant: QAPISchemaVariant) -> TreeValue:
             'case': variant.name,
             'type': self._use_type(variant.type)
         }
-        return _make_tree(obj, variant.ifcond, None)
+        return _make_tree(obj, variant.ifcond)
 
     def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
                            json_type: str) -> None: