[Qemu-devel] [PATCH v2 09/10] qmp.py: Avoid overriding a builtin object

Lukáš Doktor posted 10 patches 8 years, 6 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 09/10] qmp.py: Avoid overriding a builtin object
Posted by Lukáš Doktor 8 years, 6 months ago
The "id" is a builtin method to get object's identity and should not be
overridden. This might bring some issues in case someone was directly
calling "cmd(..., id=id)" but I haven't found such usage on brief search
for "cmd\(.*id=".

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
---
 scripts/qmp/qmp.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index f2f5a9b..ef12e8a 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
             print >>sys.stderr, "QMP:<<< %s" % resp
         return resp
 
-    def cmd(self, name, args=None, id=None):
+    def cmd(self, name, args=None, cmd_id=None):
         """
         Build a QMP command and send it to the QMP Monitor.
 
         @param name: command name (string)
         @param args: command arguments (dict)
-        @param id: command id (dict, list, string or int)
+        @param cmd_id: command id (dict, list, string or int)
         """
         qmp_cmd = {'execute': name}
         if args:
             qmp_cmd['arguments'] = args
-        if id:
-            qmp_cmd['id'] = id
+        if cmd_id:
+            qmp_cmd['id'] = cmd_id
         return self.cmd_obj(qmp_cmd)
 
     def command(self, cmd, **kwds):
-- 
2.9.4


Re: [Qemu-devel] [PATCH v2 09/10] qmp.py: Avoid overriding a builtin object
Posted by Eduardo Habkost 8 years, 6 months ago
On Tue, Jul 25, 2017 at 05:09:50PM +0200, Lukáš Doktor wrote:
> The "id" is a builtin method to get object's identity and should not be
> overridden. This might bring some issues in case someone was directly
> calling "cmd(..., id=id)" but I haven't found such usage on brief search
> for "cmd\(.*id=".
> 
> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>

I don't see the benefit of the patch, as the function is very
short and unlikely to ever use the id() builtin.  But I am not
against it if it will silence code analyzer warnings.


> ---
>  scripts/qmp/qmp.py | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index f2f5a9b..ef12e8a 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
>              print >>sys.stderr, "QMP:<<< %s" % resp
>          return resp
>  
> -    def cmd(self, name, args=None, id=None):
> +    def cmd(self, name, args=None, cmd_id=None):
>          """
>          Build a QMP command and send it to the QMP Monitor.
>  
>          @param name: command name (string)
>          @param args: command arguments (dict)
> -        @param id: command id (dict, list, string or int)
> +        @param cmd_id: command id (dict, list, string or int)
>          """
>          qmp_cmd = {'execute': name}
>          if args:
>              qmp_cmd['arguments'] = args
> -        if id:
> -            qmp_cmd['id'] = id
> +        if cmd_id:
> +            qmp_cmd['id'] = cmd_id
>          return self.cmd_obj(qmp_cmd)
>  
>      def command(self, cmd, **kwds):
> -- 
> 2.9.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 09/10] qmp.py: Avoid overriding a builtin object
Posted by Lukáš Doktor 8 years, 6 months ago
Dne 25.7.2017 v 21:34 Eduardo Habkost napsal(a):
> On Tue, Jul 25, 2017 at 05:09:50PM +0200, Lukáš Doktor wrote:
>> The "id" is a builtin method to get object's identity and should not be
>> overridden. This might bring some issues in case someone was directly
>> calling "cmd(..., id=id)" but I haven't found such usage on brief search
>> for "cmd\(.*id=".
>>
>> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
> 
> I don't see the benefit of the patch, as the function is very
> short and unlikely to ever use the id() builtin.  But I am not
> against it if it will silence code analyzer warnings.
> 
For me the biggest benefit is it decreases the probability of someone copying the same "idea" to other pieces of code, because, you know, developers are using copy&paste way too often.

Anyway pylint does complain about this issue. I can either ignore it, skip it with an informative comment `# use id for backward compatibility pylint: disable=W0622` or use this patch to fix it properly. I'm inclined towards fixing it (until it poisons other parts of the code), you are in favor of ignoring it, so let's see whether someone else has another opinion, otherwise I'll remove it in v3.

Lukáš

> 
>> ---
>>  scripts/qmp/qmp.py | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> index f2f5a9b..ef12e8a 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
>>              print >>sys.stderr, "QMP:<<< %s" % resp
>>          return resp
>>  
>> -    def cmd(self, name, args=None, id=None):
>> +    def cmd(self, name, args=None, cmd_id=None):
>>          """
>>          Build a QMP command and send it to the QMP Monitor.
>>  
>>          @param name: command name (string)
>>          @param args: command arguments (dict)
>> -        @param id: command id (dict, list, string or int)
>> +        @param cmd_id: command id (dict, list, string or int)
>>          """
>>          qmp_cmd = {'execute': name}
>>          if args:
>>              qmp_cmd['arguments'] = args
>> -        if id:
>> -            qmp_cmd['id'] = id
>> +        if cmd_id:
>> +            qmp_cmd['id'] = cmd_id
>>          return self.cmd_obj(qmp_cmd)
>>  
>>      def command(self, cmd, **kwds):
>> -- 
>> 2.9.4
>>
> 


Re: [Qemu-devel] [PATCH v2 09/10] qmp.py: Avoid overriding a builtin object
Posted by Eduardo Habkost 8 years, 6 months ago
On Wed, Jul 26, 2017 at 06:46:30AM +0200, Lukáš Doktor wrote:
> Dne 25.7.2017 v 21:34 Eduardo Habkost napsal(a):
> > On Tue, Jul 25, 2017 at 05:09:50PM +0200, Lukáš Doktor wrote:
> >> The "id" is a builtin method to get object's identity and should not be
> >> overridden. This might bring some issues in case someone was directly
> >> calling "cmd(..., id=id)" but I haven't found such usage on brief search
> >> for "cmd\(.*id=".
> >>
> >> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
> > 
> > I don't see the benefit of the patch, as the function is very
> > short and unlikely to ever use the id() builtin.  But I am not
> > against it if it will silence code analyzer warnings.
> > 
> For me the biggest benefit is it decreases the probability of
> someone copying the same "idea" to other pieces of code,
> because, you know, developers are using copy&paste way too
> often.
> 
> Anyway pylint does complain about this issue. I can either
> ignore it, skip it with an informative comment `# use id for
> backward compatibility pylint: disable=W0622` or use this patch
> to fix it properly. I'm inclined towards fixing it (until it
> poisons other parts of the code), you are in favor of ignoring
> it, so let's see whether someone else has another opinion,
> otherwise I'll remove it in v3.

If pylint complains about it, it won't hurt to make it happy.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo