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

fix cache_mode_fn consistency #65

Merged
merged 4 commits into from
Jan 5, 2024
Merged

Conversation

matt-phylum
Copy link
Contributor

cache_mode_fn was added to HttpCacheOptions to allow different cache modes to be used for different requests. However, cache_mode_fn was only consulted when determining if a request could be satisfied from the cache, not when determining whether to use an existing cached value or whether to cache a fresh value. The test was incorrectly passing because cache_mode_fn was setting the mode equal to the same value that it would have been otherwise.

This PR makes all code paths use cache_mode_fn consistently and fixes the cache_mode_fn test to rely on cache_mode_fn working.

@matt-phylum matt-phylum changed the title fix cache_mode_fn consistency fix cache_mode_fn consistency Jan 5, 2024
@06chaynes 06chaynes changed the base branch from main to develop January 5, 2024 15:12
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c1e8b6b) 92.58% compared to head (937e587) 92.64%.
Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #65      +/-   ##
===========================================
+ Coverage    92.58%   92.64%   +0.06%     
===========================================
  Files           11       11              
  Lines         1092     1101       +9     
===========================================
+ Hits          1011     1020       +9     
  Misses          81       81              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@06chaynes
Copy link
Owner

Ah good catch, and thank you for the PR! I'll try to get this in and published this weekend.

@06chaynes
Copy link
Owner

06chaynes commented Jan 5, 2024

Ignore that coverage error, it's super annoying about it. Thanks for the clippy fixes!

Edit: Ha looks like clippy was a bit overzealous with that fix I'm guessing? I can add an ignore for this case if so

@matt-phylum
Copy link
Contributor Author

I changed the wrong instance of try_into(). My IDE does not work on this project because it tries to use multiple async implementations.

@06chaynes 06chaynes merged commit 598feb7 into 06chaynes:develop Jan 5, 2024
40 checks passed
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.

None yet

2 participants