python-cachier / cachier

Persistent, stale-free, local and cross-machine caching for Python functions.
MIT License
542 stars 60 forks source link

refactor: clarify the casher arguments #198

Closed Borda closed 7 months ago

Borda commented 7 months ago

Adding prefixes similar to scikit-learn pipelines does clarify which arguments are for the function and which are for the wrapper... :flamingo: Moreover we can adapt the updating default so almost all arguments can be overwritten with a particular function call :magic_wand:

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.89%. Comparing base (2a4d1c9) to head (8e7fce5). Report is 1 commits behind head on master.

:exclamation: Current head 8e7fce5 differs from pull request most recent head acbbd64. Consider uploading reports for the commit acbbd64 to get more accurate results

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/python-cachier/cachier/pull/198/graphs/tree.svg?width=650&height=150&src=pr&token=fhsTDs7HL9&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier)](https://app.codecov.io/gh/python-cachier/cachier/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier) ```diff @@ Coverage Diff @@ ## master #198 +/- ## ========================================== + Coverage 97.84% 97.89% +0.04% ========================================== Files 8 8 Lines 511 523 +12 Branches 88 91 +3 ========================================== + Hits 500 512 +12 Misses 10 10 Partials 1 1 ``` | [Files](https://app.codecov.io/gh/python-cachier/cachier/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier) | Coverage Δ | | |---|---|---| | [cachier/config.py](https://app.codecov.io/gh/python-cachier/cachier/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier#diff-Y2FjaGllci9jb25maWcucHk=) | `100.00% <100.00%> (ø)` | | | [cachier/core.py](https://app.codecov.io/gh/python-cachier/cachier/pull/198?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier#diff-Y2FjaGllci9jb3JlLnB5) | `100.00% <100.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/python-cachier/cachier/pull/198?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/python-cachier/cachier/pull/198?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier). Last update [90488c9...acbbd64](https://app.codecov.io/gh/python-cachier/cachier/pull/198?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-cachier).
shaypal5 commented 7 months ago

This is a major API breaking change, which will require the release of a new major version. It's sensible, but it means any new improvement coming after it won't be enjoyed by anyone not going back and changing their code.

Let's finish first with everything else you have up your sleeve, and then consider it again.

Borda commented 7 months ago

This is a major API breaking change, which will require the release of a new major version.

yes, so I can preserve the actual with raising a deprecation warning and enabling the new naming, what do you think?

shaypal5 commented 7 months ago

Yes, keeping both with a deprecation warning is a good idea for now.

Borda commented 7 months ago

Yes, keeping both with a deprecation warning is a good idea for now.

cool, added related tests that these "old" args still work and deprecation is raised

shaypal5 commented 7 months ago

@Borda Same here. Conflicts must be resolved.

Borda commented 7 months ago

Conflicts must be resolved.

done :)