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

Cache default(TResult) values #559

Merged
merged 3 commits into from
Jan 26, 2019
Merged

Cache default(TResult) values #559

merged 3 commits into from
Jan 26, 2019

Conversation

cmeeren
Copy link
Contributor

@cmeeren cmeeren commented Jan 11, 2019

Fixes #523

Details on the issue fix or feature implementation

I have not added any new unit tests, because I think it's adequately covered by the numerous adapted tests.

Confirm the following

  • I have merged the latest changes from the dev vX.Y branch
  • I have successfully run a local build
  • I have included unit tests for the issue/feature
  • I have targeted the PR against the latest dev vX.Y branch

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 11, 2019

Please review carefully, these are non-trivial changes to both code and tests.

Also, feel free to merge and make changes yourself if that's quicker/easier than having me update the PR.

@reisenberger
Copy link
Member

Thanks @cmeeren . PR looks great! I am going to push four more tests which prove the default(TResult) cases work for an arbitrary cache provider (in other words, that Polly itself does not impose any limitations), for both get and put, for both value types and reference types.

Individual cache providers may of course impose their own limitations on caching null, but none (is my first initial guess...) will impose any limitation on caching default(TResult) for a value type.

Note: I am holding (not merging) this PR until we have ready the associated fixes to Polly cache providers and serialization providers:

But otherwise I think this PR (with extra tests I'll push) is good to go.

@reisenberger
Copy link
Member

@cmeeren I'm merging this into the v7.0.0 dev branch now, as I've brought the cache providers and serializers into alignment with the v7.0 changes, in the following PRs:
(the PRs on those repos will not actually be merged until Polly v7.0.0 releases publicly)

Please feel free to review those PRs. Each cache provider repo now has integration tests proving we can round-trip cache or serialize a range of values across a range of types, including the default(TResult) case for value types which you originally raised.

Thanks for your contribution! You have credit in the readme!

@reisenberger reisenberger merged commit 9808690 into App-vNext:v700 Jan 26, 2019
@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 26, 2019

Thanks, happy to help! :)

@cmeeren cmeeren deleted the cache-default branch January 26, 2019 15:35
@reisenberger
Copy link
Member

@cmeeren The change will release publicly to nuget when v7.0.0 releases. (Remaining work: documentation on the new custom policies and Simmy.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants