Skip to content

gh-64373: Convert _functools to Argument Clinic #96640

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

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 7, 2022

A couple of extra notes:

  • functools.reduce is not converted, because its args parsing / in-place mutation is optimized
  • partial_new and partial_call require *args, **kwargs, as far as I know AC does not support that at the moment, they are not converted
  • The same for _lru_cache_wrapper.__call__
  • I've also noticed a couple of other oddities that I will report in two separate issue later

@sobolevn
Copy link
Member Author

sobolevn commented Sep 7, 2022

Now, let's compare help() before and after!

cmp_to_key

Before:

>>> help(_functools.cmp_to_key)
Help on built-in function cmp_to_key in module _functools:

cmp_to_key(...)
    Convert a cmp= function into a key= function.

After:

>>> help(_functools.cmp_to_key)
Help on built-in function cmp_to_key in module _functools:

cmp_to_key(mycmp)
    Convert a cmp= function into a key= function.

    mycmp
      Function that compares two objects.

_lru_cache_wrapper

Before:

>>> help(_functools._lru_cache_wrapper)
Help on class _lru_cache_wrapper in module functools:

class _lru_cache_wrapper(builtins.object)
 |  Create a cached callable that wraps another function.
 |
 |  user_function:      the function being cached
 |
 |  maxsize:  0         for no caching
 |            None      for unlimited cache size
 |            n         for a bounded cache
 |
 |  typed:    False     cache f(3) and f(3.0) as identical calls
 |            True      cache f(3) and f(3.0) as distinct calls
 |
 |  cache_info_type:    namedtuple class with the fields:
 |                          hits misses currsize maxsize
 |
 |  Methods defined here:

After:

>>> help(_functools._lru_cache_wrapper)
Help on class _lru_cache_wrapper in module functools:

class _lru_cache_wrapper(builtins.object)
 |  _lru_cache_wrapper(user_function, maxsize, typed, cache_info_type)
 |
 |  Create a cached callable that wraps another function.
 |
 |  user_function
 |    the function being cached
 |  maxsize
 |    0         for no caching
 |    None      for unlimited cache size
 |    n         for a bounded cache
 |  typed
 |    False     cache f(3) and f(3.0) as identical calls
 |    True      cache f(3) and f(3.0) as distinct calls
 |  cache_info_type
 |    namedtuple class with the fields:
 |    hits misses currsize maxsize
 |
 |  Methods defined here:

.cache_clear and .cache_info

Before:

>>> help(_functools._lru_cache_wrapper.cache_clear)
Help on method_descriptor:

cache_clear(...)

>>> help(_functools._lru_cache_wrapper.cache_info)
Help on method_descriptor:

cache_info(...)

After:

>>> help(_functools._lru_cache_wrapper.cache_clear)
Help on method_descriptor:

cache_clear(self, /)
    Clear the cache and cache statistics

>>> help(_functools._lru_cache_wrapper.cache_info)
Help on method_descriptor:

cache_info(self, /)
    Report cache statistics

@rhettinger
Copy link
Contributor

rhettinger commented Sep 7, 2022

We generally only apply argclinic to functions directly visible to the user. Otherwise, it is mostly pointless since no one sees the signature. So, I would leave the lru cache internals alone -- iirc, we had an issue for this previously. It was would be a lot of code churn for no benefit. The cmp_to_key() part can stay.

@sobolevn
Copy link
Member Author

sobolevn commented Sep 7, 2022

Thank you for the insight! This is my first clinic-related PR.

I will remove things like __copy__ and other internals tomorrow 👍

to functions directly visible to the user
I would leave the lru cache internals alone

I think that .clear_cache and .cache_info are user-facing functions, aren't they?

@rhettinger
Copy link
Contributor

I think that .clear_cache and .cache_info are user-facing functions, aren't they?

You can change those two zero-argument methods if you want.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@sobolevn
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@rhettinger: please review the changes made to this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants