Dne 21.7.2017 v 20:46 Eduardo Habkost napsal(a): > On Fri, Jul 21, 2017 at 08:53:49AM +0200, Lukáš Doktor wrote: >> Dne 20.7.2017 v 20:38 Eduardo Habkost napsal(a): >>> On Thu, Jul 20, 2017 at 06:28:13PM +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 <ldok...@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 a14b001..c3e0206 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['cmd_id'] = cmd_id >>> >>> The member sent through the monitor should still be called "id". >>> i.e.: >>> >>> qmp_cmd['id'] = cmd_id >>> >> Oups, sorry, automatic rename changed it and I forgot to fix >> this one back. I'll address this in v2. The main problem with >> this patch is it could break named arguments (`cmd(..., id=id)` >> calls) so I'm not sure it's worth including. But as mentioned >> in commit message I grepped full sources so we better fix this >> before someone starts using it. > > I'm not convinced we need this patch, either. What exactly > breaks if we don't apply this patch and somebody tries to use > cmd(..., id=id)? >
It'll work properly. The only issue is the `id` method will be unavailable inside that function. So let's say you want to add `print` method to debug issue with duplicate arguments, you'd be unable to use `id` to distinguish between them. The other issue is that people will see it and copy&paste this ugliness to other projects poisoning the world :-) Therefor I'd like to include it, but I'm prepared to yield. Lukáš
signature.asc
Description: OpenPGP digital signature