Re: [PATCH v2 06/12] python: upgrade to python3.9+ syntax

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On Tue, Jun 24, 2025 at 3:34 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This patch is fully automated, using pymagic, isort and autoflake.
>> >
>> > Create a script named pymagic.sh:
>> >
>> > =========================
>> >
>> > pyupgrade --exit-zero-even-if-changed --keep-percent-format \
>> >           --py39-plus "$@"
>> >
>> > autoflake -i "$@"
>> >
>> > isort --settings-file python/setup.cfg \
>> >       -p compat -p qapidoc_legacy -p iotests -o qemu "$@"
>> > =========================
>> >
>> > Then, from qemu.git root:
>> >
>> >> find . -type f -name '*.py' | xargs pymagic
>> >> git grep --name-only "#!/usr/bin/env python" | xargs pymagic
>> >
>> > This changes a lot of old Pythonisms, but in particular it upgrades the
>> > old Python type hint paradigm to the new 3.9+ paradigm wherein you no
>> > longer need to import List, Dict, Tuple, Set, etc from the Typing module
>> > and instead directly subscript the built-in types list, dict, tuple,
>> > set, etc. The old-style annotations are deprecated as of 3.9 and are
>> > eligible for removal starting in Python 3.14, though the exact date of
>> > their removal is not yet known.
>> >
>> > pyupgrade updates the imports and type hint paradigms (as well as
>> > updating other old 'isms, such as removing the unicode string
>> > prefix). autoflake in turn then removes any unused import statements,
>> > possibly left behind by pyupgrade. Lastly, isort fixes the import order
>> > and formatting to the standard we use in qemu.git/python and
>> > scripts/qapi in particular.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> [...]
>>
>> >  448 files changed, 1959 insertions(+), 1631 deletions(-)
>>
>> *Ächz*
>>
>
> Gesundheit.
>
>
>>
>> I hate it when people ask me to split up my mechanical patches...
>>
>> One split is by subsystem / maintainer.  I've done this a few times, and
>> it's quite a bother.  Questionable use of your time if you ask me.
>>
>
> I'd prefer not to unless it is requested of me specifically. I don't think
> most maintainers really care about the nuances of Python and as long as
> their stuff continues to work they're not going to mind much.
>
> Or, to be frank: I don't think this series would ever garner enough review
> and attention to warrant the labor it'd take to tailor it to such a review.
> It's mechanical, it's boring, it should be fine.
>
> I switched from a manual patch series to a tool-driven one specifically to
> make it more mindless and less interesting, and going through and splitting
> it back out is ... eh. I would prefer not to.
>
>
>>
>> There's another split here...  Your pymagic.sh runs three tools.  If you
>> commit after each one, the patch splits into three.
>>
>
> I use all three because each one alone isn't sufficient to then pass the
> static analysis checks, they each do a little bit of damage that another
> tool corrects afterwards.
>
> pyupgrade works to modernize syntax, but leaves impotent import statements
> hanging.

Import statements it made impotent, I presume.

> autoflake removes those impotent imports.
> isort fixes the import statement ordering and formatting to our standard.

Out of curiosity: what messes up ordering and formatting?

> (And then I do some manual fixups to fix the linting tests where things
> were auto-formatted suboptimally.)
>
> I can still split it out for review purposes, like I did here with some
> manual fixups appended to the end.
>
> Just, for merge, they'll be combined by necessity as a result of our
> no-regressions-for-bisect rule.

I see.

>> I understand you pass --py39-plus to pyupgrade to get the type hints
>> modernized.  If you run it without --py39-plus for all the miscellaneous
>> upgrades, commit, then run it with --py39-plus for just the type hint
>> upgrades, commit, the last patch splits again.
>>
>
> I can try it! I actually didn't try running it without py39-plus at all, so
> I don't know what that'll do. but no harm in an experiment.
>
>
>>
>> Thoughts?
>
>
> First and foremost I just thought it'd be good to get this mechanical
> change squared away in one giant patch so we could add this one singular
> horrible mega-commit into the git blame "ignored commits" list to minimize
> the impact of the "flag day".

Point.

Still, it's awfully hard to see what the horrible mega-commit does.

A patch that does one thing entirely mechanically is fine even when it's
huge.  Understanding the one thing is easy.  I can usually develop
confidence in the patch.

A patch that does many things mechanically can be problematic.  If it's
small enough, I can just review them like any other patch.  If it's way
too big for that, we have to rely on appeal to authority, i.e. the
tool(s) that generated the patch.  Certainly not nothing, but it gives
me an uneasy feeling.

This is why I'm keen to see the type hint upgrade split off.  I expect
the type hint part to do one thing entirely mechanically (fine), and I
hope the other part will be small enough to let me build confidence in
it.

> This upgrade will have to happen "eventually" but it needn't be "right
> now", but I figured it'd be good to get it out of the way... or put another
> way, "better my mess than someone else's".

I'd prefer upgrade now rather than later for the Python code I maintain.