-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-104683: Argument clinic: cleanup state_modulename_name()
#107340
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
Conversation
This is a very nice improvement, indeed. A lot of the error paths in this PR don't have test coverage, yet. IMO, we should bring test coverage of |
Thanks! |
(Sorry, I forgot to mention: we should add tests in separate PRs, since we normally want those backported to bugfix branches.) |
In general, I agree -- but here, adding tests revealed some micro-bugs in the failure messages, so I would have had to assert arguably incorrect behaviour in the tests in order to backport them to bugfix branches. Unless we consider the microbugs in the failure messages important enough to warrant backporting this PR as a bugfix, which I personally don't? Anyway, that was my thought process :) |
The
state_modulename_name()
function has two variables namedmodule
, that have different purposes and that are assigned to objects of different types:cpython/Tools/clinic/clinic.py
Line 4718 in 6e850c3
cpython/Tools/clinic/clinic.py
Lines 4746 to 4750 in 6e850c3
If we give the second variable a different name, it makes the code easier to understand for mypy and for humans. I also made some incidental changes to nearby lines to use f-strings rather than string concatenation using
+
.(This PR is required in order to add type hints to the last remaining untyped function in
clinic.py
,_module_and_class()
.)Tools/clinic/
#104683