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

UP011 is incorrectly replacing lru_cache(max_size)=None #1913

Closed
henribru opened this issue Jan 16, 2023 · 10 comments · Fixed by #1918
Closed

UP011 is incorrectly replacing lru_cache(max_size)=None #1913

henribru opened this issue Jan 16, 2023 · 10 comments · Fixed by #1918
Assignees
Labels
bug Something isn't working

Comments

@henribru
Copy link

henribru commented Jan 16, 2023

The UP011 rule replaces functools.lru_cache(max_size=None) with functools.lru_cache. The corresponding rule in pyupgrade replaces it with functools.cache: https://github.com/asottile/pyupgrade#replace-functoolslru_cachemaxsizenone-with-shorthand.

Tested with ruff 0.0.222

@charliermarsh charliermarsh added the bug Something isn't working label Jan 16, 2023
@charliermarsh charliermarsh self-assigned this Jan 16, 2023
@charliermarsh
Copy link
Member

Thank you! Fixing now. Just an oversight.

@henribru
Copy link
Author

@charliermarsh Thanks for the super quick response! Your fix seems to work fine for the @functools.lru_cache(max_size=None) case, but there's still a slight issue if you do from functools import lru_cache and then @lru_cache(max_size=None). In that case it doesn't auto-fix anything, but it still suggests "Remove unnecessary parameters"

@charliermarsh
Copy link
Member

@henribru - Yeah this is a known limitation (and pyupgrade doesn't fix that case either, IIRC). In that case, we actually need to modify the import too, to include cache. So for now, we flag it, but can't fix it.

Another case of #835.

@henribru
Copy link
Author

henribru commented Jan 17, 2023

@charliermarsh That's a fair limitation, but still the message is pretty misleading since it's telling you to remove the parameters instead of changing lru_cache to cache. Since just removing max_size=None changes the behavior, it's not "unnecessary"

@henribru
Copy link
Author

Note that pyupgrade separates this into two rules, one that converts lru_cache() into lru_cache and one that converts lru_cache(max_size=None) into cache. The current error message works for the first case, but it seems misleading for the second

@charliermarsh
Copy link
Member

Ah right -- I'll modify the error message.

@charliermarsh
Copy link
Member

(And consider splitting into two rules, too.)

@charliermarsh
Copy link
Member

Fixed in #1938.

@henribru
Copy link
Author

Thanks again for all your hard work, I really appreciate it!

@charliermarsh
Copy link
Member

Thanks for being persistent on this one even after I closed it out -- you were totally right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants