gh-113317: Change how Argument Clinic lists converters
- Add a new create_python_parser_namespace() function for PythonParser to pass objects to executed code.
- In run_clinic(), list converters using 'converters' and 'return_converters' dictionarties.
- test_clinic: add 'object()' return converter.
- Issue: gh-113317
I created https://github.com/python/cpython/pull/116836 but then realized that it's too big, so I created this PR for changes which are not just moving code around.
@erlend-aasland: Would you mind to review it?
@erlend-aasland: Would you mind to review the updated PR?
Could you try to not rebase and force-push? It is unconvenient to track the PR locally when the branches get out of sync all the time. Moreover, we do tell all our new contributors to not force-push[^1][^2], so I think core devs should set a good example and play by the book 🙂
[^1]: it's even documented in the devguide [^2]: because of all the inconveniencies wrt. reviewing, amongst other things ...
I updated the PR:
- I added a cache to create_parser_namespace(): since the function is called often and the dict only needs to be created once. Then we can just copies.
- I reused create_parser_namespace() in eval_ast_expr() which used
globalsbefore.
Could you try to not rebase and force-push?
Oh sorry. I had to use git push --force, since I made multiple mistakes :-( I didn't expect anymore to look at my PR while I'm modifying. I will avoid rebase/amend.
This tuple item seems to be unused. Ditto for the return converter list.
The return converter list is used by the clinic.py --converters command. Try to remove it. See:
for title, attribute, ids in (
("Converters", 'converter_init', converter_list),
("Return converters", 'return_converter_init', return_converter_list),
):
You modified the code to use:
converter_list: list[tuple[str, str, ConverterType]] = []
return_converter_list: list[tuple[str, str, ReturnConverterType]] = []
That was my first attempt, but it makes mypy grumpy. I updated the PR to use ConverterType | ReturnConverterType instead: now it works.
@erlend-aasland: Would you mind to review the updated PR?
I think I missed how the parser namespace change interacts with how Argument Clinic lists converters; could you point it out again? Also, should the new global functions be put into libclinic? If so, would a parser.py file be a fitting home? What do you think?
I think I missed how the parser namespace change interacts with how Argument Clinic lists converters; could you point it out again?
Without this change, converters and return converters cannot be moved out of clinic.py.
Also, should the new global functions be put into libclinic? If so, would a parser.py file be a fitting home? What do you think?
create_parser_namespace() cannot be moved out of clinic.py before converters are moved.
I think I missed how the parser namespace change interacts with how Argument Clinic lists converters; could you point it out again?
Without this change, converters and return converters cannot be moved out of
clinic.py.Also, should the new global functions be put into libclinic? If so, would a parser.py file be a fitting home? What do you think?
create_parser_namespace() cannot be moved out of clinic.py before converters are moved.
Thanks! It's hard for me to context switch between RL and CPython these days :)
Merged, thanks for the review.
See #117347 for a possible regression.