Skip to content

Convert signatures to keyword-only arguments#338

Closed
ddelange wants to merge 1 commit intotkem:masterfrom
ddelange:keyword-only
Closed

Convert signatures to keyword-only arguments#338
ddelange wants to merge 1 commit intotkem:masterfrom
ddelange:keyword-only

Conversation

@ddelange
Copy link
Contributor

@ddelange ddelange commented Apr 3, 2025

Hi @tkem 👋

Hoping this PR can land in v6 🤞

This PR:

  • Reduces bug surface by using keyword-only arguments (PEP-3102) in most signatures.
  • Makes condition, lock, info configurable for the cachetools.func module.
    • Switches from info=True default to info=False default analogous to the main module.
  • Deprecates dummy_threading which has been unavailable since 3.7 (the minimum required version of cachetools in setup.cfg).
    • by the way, why is CI only running 3.9+?

Comment on lines -603 to -612
if isinstance(condition, bool):
from warnings import warn

warn(
"passing `info` as positional parameter is deprecated",
DeprecationWarning,
stacklevel=2,
)
info = condition
condition = None
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the typical type of bug that becomes impossible to make with keyword-only arguments

@ddelange ddelange force-pushed the keyword-only branch 14 times, most recently from f862218 to 9f4d99d Compare April 3, 2025 16:48
@ddelange ddelange marked this pull request as ready for review April 3, 2025 16:54
@ddelange ddelange force-pushed the keyword-only branch 3 times, most recently from 9a090f2 to 41e3fc4 Compare April 3, 2025 18:00
@ddelange
Copy link
Contributor Author

ddelange commented Apr 3, 2025

This PR is green now on my fork 👍

@ddelange
Copy link
Contributor Author

ddelange commented Apr 4, 2025

Hi @tkem 👋

This is ready for review

@tkem
Copy link
Owner

tkem commented Apr 7, 2025

@ddelange: Thanks for your interest!

Without - i have to admit - looking too deeply into your submissions:

  • Reduces bug surface by using keyword-only arguments: Good idea in principle, but a possibly (though probably unlikely) breaking change. I won't introduce something like this without giving at least six months notice with DeprecationWarning. So, yes, that was already on my list, but not for 6.0.
  • Makes condition, lock, info configurable for the cachetools.func module: cachetools.func intentionally follows functools.lru_cache , and tries to stay as simple as possible for the user. Therefore, the choice to omit lock, condition, and info was deliberate, and will not change in the futue.
  • Deprecates dummy_threading which has been unavailable since 3.7: Good point, I guess I just missed that.

So, could you just refactor and make a seperate PR for the "dummy_threading" stuff? Would be appreciated!

@tkem tkem closed this Apr 7, 2025
@tkem
Copy link
Owner

tkem commented Apr 7, 2025

by the way, why is CI only running 3.9+?

Because it is the oldest officially supported Python version:

https://devguide.python.org/versions/

@ddelange
Copy link
Contributor Author

ddelange commented Apr 7, 2025

Makes condition, lock, info configurable for the cachetools.func module.

  • Switches from info=True default to info=False default analogous to the main module.

would it be an option to set a lock enabled by default en info disabled by default on the cachetools.func decorators? almost any use case I have for these decorators is multithreaded, essentially rendering the cachetools.func short hands useless

@ddelange
Copy link
Contributor Author

ddelange commented Apr 8, 2025

by the way, why is CI only running 3.9+?

Because it is the oldest officially supported Python version:

https://devguide.python.org/versions/

although this is true, setup-python still supports 3.7. i would recommend to either bump setup.cfg to 3.9+, or include 3.7 in CI to avoid unwelcome surprises

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.

2 participants