-
Notifications
You must be signed in to change notification settings - Fork 1.6k
A couple mypy/ruff fixes #13001
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
A couple mypy/ruff fixes #13001
Conversation
ea6e65d
to
4c25bc0
Compare
We added this job less than a week ago to account for the fact that mypy on 3.8 does, in fact, fail in places where 3.13 doesn't. See 1be08b9. As a result, this PR regresses our CI. Given this job is incredibly fast, I don't see a need to make changes here. |
I don't think it's "mypy on 3.8" failing, instead it's mypy 1.14 failing. What's the point of running mypy on Python 3.8 and running mypy 1.14 instead of 1.15? I mean, while Python 3.8 is a supported target platform of cryptography, what is the point of supporting an EOL version of Python for running static linting tools, given this has no impact on the runtime behaviour of cryptography under that version of Python? Additionally, the purpose of this PR is not speed, the intent was to get rid of this CI warning:
|
I don't believe that's correct. As the original issue describes, the issue that this test was added to catch reproduces on 3.9, which supports mypy 1.15. (You'll also see that the changelog for mypy 1.15 doesn't mention this feature: https://mypy-lang.blogspot.com/2025/02/mypy-115-released.html). The point of this test is to verify that users will be able to typecheck their programs against cryptography. AFAIK there's no way to resolve that warning without regressing this. |
b579a8c
to
4a8e841
Compare
I get it now. While there are no code path differences in cryptography itself, there may be differences in modules such as Now, if it's about Python 3.9, the way to resolve the warning is to run mypy with Python 3.9 and 3.13 instead of 3.8 and 3.13, as they all support 1.15. But again, I don't see a fundamental reason to run mypy with Python 3.9 instead of a more recent version: it won't catch runtime issues of cryptography specific to that version of Python, will it? The practical reason is that developers may be stuck on a platform with older default versions of Python (I am myself stuck on an Ubuntu 20.04 server with Python 3.8 at times). I wonder whether |
It's about supporting all the configurations we expect to work for our users. I do not understand what your motivation is for this -- at this point it's consuming a lot of my time to explain this, and the upside is effectively nothing. |
Are they expected to work for end-users of the cryptography module or developers/users that run mypy on the source code? That's the distinction I am making above. |
Both. |
My point was that it's not useful to let developers run mypy under Python 3.8. However, after reading Issues with code at runtime, I realise there may be runtime issues after all:
I wonder whether these might be addressed using Any way, let me read more about it and stop annoying you. I'll modify this PR to apply three very simple changes:
|
Option `warn_unused_configs` requires turning off incremental mode: https://mypy.readthedocs.io/en/stable/config_file.html#confval-warn_unused_configs
No need to specify it explicitly, the same way we don't specifiy it with mypy or sidst-check.
4a8e841
to
18d5ea5
Compare
Fixes #13000.Supersedes #12989.EDIT:
pyproject,toml
to match versions inci-constraints-requirements.txt
.warn-unused-configs = true
requiresincremental = false
according to the--warn-unused-configs
documentation: