-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-104050: Annotate more Argument Clinic DSLParser state methods #106376
gh-104050: Annotate more Argument Clinic DSLParser state methods #106376
Conversation
@AlexWaygood, can you help me with the last bits on this PR? |
Taking a look now :) |
This diff fixes the first two mypy errors: --- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -4846,7 +4846,9 @@ def state_parameter(self, line: str | None) -> None:
if not module:
fail("Function " + self.function.name + " has an invalid parameter declaration:\n\t" + line)
- function_args = module.body[0].args
+ function = module.body[0]
+ assert isinstance(function, ast.FunctionDef)
+ function_args = function.args
if len(function_args.args) > 1:
fail("Function " + self.function.name + " has an invalid parameter declaration (comma?):\n\t" + line)
@@ -4931,7 +4933,9 @@ def bad_node(self, node):
if bad:
fail("Unsupported expression as default value: " + repr(default))
- expr = module.body[0].value
+ assignment = module.body[0]
+ assert isinstance(assignment, ast.Assign)
+ expr = assignment.value
# mild hack: explicitly support NULL as a default value
c_default: str | None
if isinstance(expr, ast.Name) and expr.id == 'NULL': In both cases, mypy is flagging that, in principle, an cpython/Tools/clinic/clinic.py Lines 4849 to 4850 in 71b4044
For the second one, the source code that we're parsing is generated here: cpython/Tools/clinic/clinic.py Lines 4902 to 4904 in 71b4044
So we can be 100% confident that in the first case, This diff fixes the last mypy error: --- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -4970,7 +4974,7 @@ def bad_node(self, node):
else:
value = ast.literal_eval(expr)
py_default = repr(value)
- if isinstance(value, (bool, None.__class__)):
+ if isinstance(value, (bool, NoneType)):
c_default = "Py_" + py_default
elif isinstance(value, str):
c_default = c_repr(value) Type checkers do a lot of special casing around |
Of course, the |
Thanks! |
No problem at all! 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Off-topic: this function is way too large and complicated, and desperately in need of modernisation as well. I had a go a while back (main...AlexWaygood:cpython:clinic-asts), but some tests were failing... that branch is also quite out of date now :-) And test coverage was poor, IIRC.
But maybe I'll get back to it at some point...
* main: pythongh-106368: Increase Argument Clinic test coverage (python#106389) pythongh-106320: Fix _PyImport_GetModuleAttr() declaration (python#106386) pythongh-106368: Harden Argument Clinic parser tests (python#106384) pythongh-106320: Remove private _PyImport C API functions (python#106383) pythongh-86085: Remove _PyCodec_Forget() declaration (python#106377) pythongh-106320: Remove more private _PyUnicode C API functions (python#106382) pythongh-104050: Annotate more Argument Clinic DSLParser state methods (python#106376) pythongh-106368: Clean up Argument Clinic tests (python#106373)
Annotate the following methods: