[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 03/22] qapi/source: Remove line number from QAPISourceInfo in
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 03/22] qapi/source: Remove line number from QAPISourceInfo initializer |
Date: |
Tue, 27 Apr 2021 08:07:24 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
John Snow <jsnow@redhat.com> writes:
> On 4/24/21 2:38 AM, Markus Armbruster wrote:
>> Mixing f-string and % interpolation. I doubt we'd write it this way
>> from scratch. I recommend to either stick to % for now (leave
>> conversion to f-strings for later), or conver the column formatting,
>> too, even though it's not related to the patch's purpose.
>
> True. Two thoughts:
>
> 1. I don't like using % formatting because it behaves differently from
> .format() and f-strings. My overwhelming desire is to never use it for
> this reason.
>
> Example: {foo} will call foo's __format__ method, whereas "%s" % foo
> will simply add str(foo). They are not always the same, not even for
> built-in Python objects.
I only care for readability, which profits from local consistency.
Maybe I'll sing a different tune once I got actually bitten by the
difference between interpolation and f-strings.
> 2. Cleaning up the formatting here without cleaning it up everywhere
> is a great way to get the patch NACKed. You have in the past been
> fairly reluctant to "While we're here" cleanups, so I am trying to cut
> back on them.
Yes, I've been pushing back on such cleanups. But it's really a
case-by-case issue. When a patch fits on a page, squashing in a bit of
losely related cleanup is usually fine. When it's long, keep it focused
on a single purpose.
> This is why my habit for f-strings keeps trickling in: whenever I have
> to rewrite any interpolation, I reach for the one that behaves most
> idiomatically for Python 3. I am trying to balance that against churn
> that's not in the stated goals of the patch.
>
> In this case: I'll clean the rest of the method to match; and add a
> note to the commit message that explains why.
Okay.
> I will get around to
> removing all of the f-strings,
The opposite, I presume.
> but I want to hit the clean linter
> baseline first to help guide the testing for such a series. I regret
> the awkward transitional period.
I'd leave converting interpolation to f-strings for later.
I can tolerate early, partial conversion, since I trust complete
conversion will happen, and as long as the resulting local inconsistency
isn't too grating. Subjective, I know.
[PATCH 04/22] qapi/parser: factor parsing routine into method, John Snow, 2021/04/21
[PATCH 08/22] qapi/parser: Use @staticmethod where appropriate, John Snow, 2021/04/21
[PATCH 06/22] qapi/parser: assert get_expr returns object in outer loop, John Snow, 2021/04/21