Re: [PATCH 5/5] qapi: delete un-needed python static analysis configs

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On Tue, Mar 25, 2025 at 4:05 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > The pylint config is being left in place because the settings differ
>> > enough from the python/ directory settings that we need a chit-chat on
>> > how to merge them O:-)
>> >
>> > Everything else can go.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>

[...]

>> > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
>> > deleted file mode 100644
>> > index 8109470a031..00000000000
>> > --- a/scripts/qapi/mypy.ini
>> > +++ /dev/null
>> > @@ -1,4 +0,0 @@
>> > -[mypy]
>> > -strict = True
>> > -disallow_untyped_calls = False
>> > -python_version = 3.8
>>
>> python/setup.cfg has:
>>
>>    [mypy]
>>    strict = True
>>    python_version = 3.8
>>    warn_unused_configs = True
>>    namespace_packages = True
>>    warn_unused_ignores = False
>>
>> Also a bunch of [mypy-FOO] sections that don't apply here.
>>
>> You explained the differences in review of a prior iteration.  Recap:
>>
>> } warn_unused_configs: Catches config values that aren't actually recognized
>> } or used. Was helpful once upon a time when re-arranging the Python
>> } directory to behave like a package to ensure that the conf files were
>> } working correctly.
>>
>> Could this be culled now?
>>
>
> Maybe!
>
>
>>
>> Hmm, according to mypy(1), strict implies warn-unused-configs.
>>
>> The question does not block this patch.
>>
>
> Send me a patch to drop it O:-)

Done:

    [PATCH] python: Drop redundant warn_unused_configs = True
    Message-ID: <20250326071203.238931-1-armbru@redhat.com>

>> } namespace_packages: Needed for the python/ directory structure (nested
>> } packages under a namespace, "qemu"). Doesn't impact scripts/qapi at all.
>> } Read up on PEP420 if you are curious. Details in commit message, see below
>> } if you're still curious.
>>
>> mypy(1) makes me suspect this is the default.  If that's true across the
>> versions we care for, this could be culled.
>>
>> Also does not block this patch.
>>
>
> It definitely wasn't once upon a time. It may still not be true on the
> oldest mypy we currently support. We don't have a clear policy for what
> versions of python libraries we need to support - this is a muddy, gray
> area. So far I just try to avoid breaking support on older versions
> needlessly, but I don't have an upgrade policy.

So what is the oldest mypy we currently support?  Unknown, best effort
to fix any breakage we see?  Wouldn't quite match my dictionary's
definition of "support"...

> If we want to integrate this directly into make check, we'll likely need to
> formalize this policy.

My gut feeling: supporting old mypy isn't worth much (if any) trouble.

>> } warn_unused_ignores: Needed once upon a time for cross-version mypy support
>> } where some versions would warn in some cases and others would not. Adding
>> } an ignore would effectively just invert which versions complained. Probably
>> } still needed, but it's hard to measure.
>>
>> Harmless enough.
>>
>> } python_version: Changes mypy behavior regardless of the invoking python
>> } interpreter to check the file as if it were to be executed by Python 3.8. I
>> } actually want to remove this value from setup.cfg but haven't yet. I
>> } removed it from the python-qemu-qmp repo and never added it for qapi.
>> } Removing it is actually probably correct as it will catch errors specific
>> } to various python versions we support, but there are some nits to iron out
>> } in my neck of the woods. This is a case where scripts/qapi/ is stricter
>> } than python/ :)
>> } (Not reasonable to solve for this series.)
>>
>> Also present in the deleted file, so no change.
>>
>> } lack of disallow_untyped_calls = False: I think this might be a remnant
>> } from when we gradually typed qapi; it's evidently no longer needed since
>> } qapi still checks fine without this affordance. The default under strict is
>> } True.
>>
>> Fair enough.
>>
>> Let's mention the differences in the commit message.  Here's my try:
>>
>>     Since the previous commit, python/setup.cfg applies to scripts/qapi/
>>     as well.  Configuration files in scripts/qapi/ override
>>     python/setup.cfg.
>>
>>     scripts/qapi/.flake8 and scripts/qapi/.isort.cfg actually match
>>     python/setup.cfg exactly, and can go.
>>
>>     The differences between scripts/qapi/mypy.ini and python/setup.cfg
>>     are harmless: [list the differences, explain why they're harmless as
>>     long as you can keep it brief, and if not, fall back to "trust me"].
>>     So scripts/qapi/mypy.ini can go, too.
>>
>>     The pylint config is being left in place because the settings differ
>>     enough from the python/ directory settings that we need a chit-chat on
>>     how to merge them O:-)
>>
>> With something like that
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>
> okey-dokey, let me integrate this feedback and I'll re-send, but I'm going
> to wait until we hash everything else out - you had some questions on other
> bits in this series.

Thanks!