Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

[Philippe Mathieu-Daudé]

On Tue, Jul 25, 2017 at 3:13 AM, Lukáš Doktor <address@hidden> wrote:
> Dne 25.7.2017 v 08:04 Philippe Mathieu-Daudé napsal(a):
>> Hi Lukáš,
>>
>> On 07/24/2017 09:36 AM, Lukáš Doktor wrote:
>>> Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
>>>> Hi Lukáš,
>>>>
>>>> Since comment/indent fixes and code changes are not related I'd rather see 
>>>> this split in at least 2 patches.
>>>>
>>> Hello Philippe, thank you for the review, I'm wondering what code changes 
>>> you have in mind? This is commit should not bring any code changes, just 
>>> code reorganization (like including the self.* attributes on top of the 
>>> file)
>>>
>>>> On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
>>>>> No actual code changes, just a few pylint/style fixes and docstring
>>>>> clarifications.
>>>>>
>>>>> Signed-off-by: Lukáš Doktor <address@hidden>
>>>>> ---
>>>>>    scripts/qmp/qmp.py | 37 ++++++++++++++++++++++++-------------
>>>>>    1 file changed, 24 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>> [...]
>>>>>        def __init__(self, address, server=False, debug=False):
>>>>>            """
>>>>>            Create a QEMUMonitorProtocol class.
>>>>> @@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
>>>>>            self.__address = address
>>>>>            self._debug = debug
>>>>>            self.__sock = self.__get_sock()
>>>>> +        self.__sockfile = None
>>
>> I was thinking about this line which is new. Now the declaration and 
>> initialization are done in __init__() while before it was only 
>> declared/initialized in connect() or accept().
>>
>> I'm not expert of python interpreter/jit internals but expect the generated 
>> code to be slightly different, even if achieving the same.
>>
>> not a bit deal, just about wording ;)
>>
> Well the difference is that before `connect` you get `AttributeError` when 
> looking for `self.__sockfile` while with this patch you'll get `None`. In 
> this code nobody queries for `self.__sockfile` before the `connect` so it 
> should not change the behavior of this code so I consider that as a `style` 
> fix as it's ugly to extend attributes later in code (with some exceptions 
> like Namespace-objects..). Anyway if you insist I can split those patches.

I'm not insisting ;) Just add something like "also initialize
__sockfile to avoid AttributeError while introspecting object before
any call to connect/accept" in the commit message and that's fine to
me.