Skip to content
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-126662: consistent naming for functools._CacheInfo #126721

Closed
wants to merge 2 commits into from

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Nov 12, 2024

This MR updates functools._CacheInfo so it's named consistently.

@rhettinger
Copy link
Contributor

Is this really necessary? The CacheInfo result is exposed to the user and this just makes it look weird. Also, changes like this tend to break doc tests.

@tungol
Copy link
Contributor Author

tungol commented Nov 12, 2024

Would you be okay with keeping the name as CacheInfo but updating the variable to match? I didn't reach for that originally because it removes the private underscore prefix, but it still wouldn't be documented or present in __all__. We can keep _CacheInfo to prevent disruption to anyone using it directly, and it would avoid display weirdness or problems with doc tests.

Is it really necessary either way? I can't say that it's necessary, but name consistency provides small benefits to some runtime and static analysis use cases.

I updated to the alternate approach - I didn't update the use of _CacheInfo as an argument name of _lru_cache_wrapper because then I'd be changing the signature. Let me know if what you think.

@rhettinger
Copy link
Contributor

I'm inclined to leave this as-is. The code has been stable for a long time and does not appear to have caused any real world problems.

ISTM that this falls in the foolish consistency category and that there isn't an actual user issue that needs to be solved. More likely, any change will break code that currently relies on what we have now, Hyrum's Law.

@tungol
Copy link
Contributor Author

tungol commented Nov 14, 2024

Okay, thanks for considering it.

@tungol tungol closed this Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants