cpython icon indicating copy to clipboard operation
cpython copied to clipboard

gh-113317: Change how Argument Clinic lists converters

Open vstinner opened this issue 1 year ago • 2 comments

  • 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

vstinner avatar Mar 15 '24 08:03 vstinner

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?

vstinner avatar Mar 15 '24 08:03 vstinner

@erlend-aasland: Would you mind to review the updated PR?

vstinner avatar Mar 21 '24 11:03 vstinner

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 ...

erlend-aasland avatar Mar 22 '24 08:03 erlend-aasland

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 globals before.

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.

vstinner avatar Mar 22 '24 08:03 vstinner

@erlend-aasland: Would you mind to review the updated PR?

vstinner avatar Mar 24 '24 15:03 vstinner

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?

erlend-aasland avatar Mar 26 '24 23:03 erlend-aasland

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.

vstinner avatar Mar 27 '24 08:03 vstinner

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 :)

erlend-aasland avatar Mar 27 '24 20:03 erlend-aasland

Merged, thanks for the review.

vstinner avatar Mar 27 '24 22:03 vstinner

See #117347 for a possible regression.

erlend-aasland avatar Mar 28 '24 23:03 erlend-aasland