Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[John Snow]

On 3/24/20 11:12 AM, Max Reitz wrote:
> On 24.03.20 16:02, Max Reitz wrote:
>> On 17.03.20 01:41, John Snow wrote:
>>> 79 is the PEP8 recommendation. This recommendation works well for
>>> reading patch diffs in TUI email clients.
>>
>> Also for my very GUI-y diff program (kompare).
>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------
>>>  tests/qemu-iotests/pylintrc   |  6 +++-
>>>  2 files changed, 47 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3d90fb157d..75fd697d77 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>
>> [...]
>>
>>> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
>>>              self.pause_drive(drive, "write_aio")
>>>              return
>>>          self.qmp('human-monitor-command',
>>> -                 command_line='qemu-io %s "break %s bp_%s"' % (drive, 
>>> event, drive))
>>> +                 command_line='qemu-io %s "break %s bp_%s"'
>>> +                 % (drive, event, drive))
>>
>> Can we put this value in a variable instead?  I don’t like the %
>> aligning with the parameter name instead of the string value.  (I also
>> don’t like how there are no spaces around the assignment =, but around
>> the %, even though the % binds more strongly.)
>>
>>>  

I think I had another patch somewhere that added an HMP helper that
fixes this issue for this particular instance.

I can send that separately as a follow-up. I think otherwise,
unfortunately, "we" "decided" that this indent style is "best".

So I think that this patch is "correct".


(All of the other options for indent styles seemed to be worse visually,
or actively go against PEP8. While PEP8 is not a bible, every conscious
choice to disregard it generally means you will be fighting a CQA tool
at some other point in time. I have therefore adopted a "When in Rome"
policy to reduce friction wherever possible with pylint, flake8, mypy,
pycharm, and so on.)


((I would prefer we begin to deprecate uses of % and begin using
.format() and f-strings wherever possible to help alleviate this kind of
syntax, too -- but I must insist that's for another series.))


>>>      def resume_drive(self, drive):
>>>          self.qmp('human-monitor-command',
>>> -                 command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
>>> drive))
>>> +                 command_line='qemu-io %s "remove_break bp_%s"'
>>> +                 % (drive, drive))
>>>  
>>>      def hmp_qemu_io(self, drive, cmd):
>>>          '''Write to a given drive using an HMP command'''
>>> @@ -793,16 +805,18 @@ def dictpath(self, d, path):
>>>                  idx = int(idx)
>>>  
>>>              if not isinstance(d, dict) or component not in d:
>>> -                self.fail('failed path traversal for "%s" in "%s"' % 
>>> (path, str(d)))
>>> +                self.fail(f'failed path traversal for "{path}" in "{d}"')
>>
>> Do we require 3.6 so that f-strings are guaranteed to work?  (I thought
>> we didn’t.  I’d be happy to use them.)
> 
> Oh.  Of course we do.  It says so in this file that this whole series is
> about.
> 

Yup. I didn't like the idea of iotests using a newer version of python
than our build system, but my concern was not shared, so we get to use
f-strings for non-buildtime tools.

End of support for Python 3.5 will be 2020-09-13; so we'll get to use
f-strings everywhere else soon, too.

--js