[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 07/11] iotests: limit line length to 79 chars
From: |
Max Reitz |
Subject: |
Re: [PATCH v8 07/11] iotests: limit line length to 79 chars |
Date: |
Tue, 24 Mar 2020 18:38:04 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
On 24.03.20 18:09, John Snow wrote:
>
>
> 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".
Perhaps it’s the best if we assume that we had to do it this way, but
can’t we just do a
qemu_io_cmd = f'qemu-io {drive} "break {event} bp_{drive}"'
self.qmp('human-monitor-command', command_line=qemu_io_cmd)
?
(Or maybe an f-string inline. I was thinking of a separate variable
because I wasn’t aware that f-strings would be an option until I got to
later hunks of this patch...)
> (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.))
Hm. But you do change something to f-strings below, why not here?
Max
signature.asc
Description: OpenPGP digital signature
- [PATCH v8 06/11] iotests: drop Python 3.4 compatibility code, (continued)
[PATCH v8 10/11] iotests: Mark verify functions as private, John Snow, 2020/03/16
[PATCH v8 08/11] iotests: add script_initialize, John Snow, 2020/03/16
[PATCH v8 11/11] iotests: use python logging for iotests.log(), John Snow, 2020/03/16