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

Order independent kwargs handling during cache key calculation #133

Conversation

levitation
Copy link

Related to issue #132

Added sorting of kwargs so that changing the ordering will not invalidate the cache.
Added test for this feature.

Note: The keyword arguments are sorted only for cache key calculation. The actual function is still called with unordered arguments. The rationale for the latter is that if somebody is debugging their code then they might be confused if the arguments they see as their function input are in a different order than expected.

@shaypal5
Copy link
Collaborator

Nice! I've read the code and test - looks good. The tests are now running. Let's wait and make sure they all pass.

@levitation
Copy link
Author

There are some test errors, but I do not quite understand them. Some are permission errors. Some tests failed in my computer even with unmodified code.

As far as I can tell, my modifications did not introduce new errors - tests that succeeded before, succeeded after modifications as well.

@levitation
Copy link
Author

I see there is some blank line at the end of file formatting error. I will try to adjust that.

Copy link

codecov bot commented Dec 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b453aa5) 100.00% compared to head (1f44781) 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #133   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          508       510    +2     
  Branches        88        88           
=========================================
+ Hits           508       510    +2     
Files Coverage Δ
cachier/core.py 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b453aa5...1f44781. Read the comment docs.

@levitation
Copy link
Author

levitation commented Dec 29, 2023

Looks like the "blank line at the end of file" formatting failure is fixed now.

The remaining test failures seem to be related to some connection error. I guess that is something I cannot control.

@shaypal5 shaypal5 changed the base branch from master to order_kwargs January 1, 2024 11:32
@shaypal5 shaypal5 merged commit 54a8506 into python-cachier:order_kwargs Jan 1, 2024
11 of 19 checks passed
@shaypal5
Copy link
Collaborator

shaypal5 commented Jan 1, 2024

Please see PR #134 .

All tests pass when testing an actual MongoDB server, but test coverage dropped under a 100%.

See details in codecov:
https://app.codecov.io/gh/python-cachier/cachier/pull/134/indirect-changes

Please add a test to cover these lines.

@levitation

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