Feat: Add support for provider hooks #38
Conversation
Codecov Report
@@ Coverage Diff @@
## main #38 +/- ##
=======================================
Coverage 99.33% 99.34%
=======================================
Files 15 16 +1
Lines 303 305 +2
Branches 24 25 +1
=======================================
+ Hits 301 303 +2
Partials 2 2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
benjiro
force-pushed
the
feat/provider-hooks
branch
2 times, most recently
from
2065df8
to
deaaaaa
Compare
- [Breaking] Remove IFeatureProvider interface infavor of FeatureProvider abstract class so default implementations can exist - Make sure Provider hooks are called in the correct order - Use strick mocking mode so sequence is validated correctly - Update test cases for provider hooks support Signed-off-by: Benjamin Evenson <2031163+benjiro@users.noreply.github.com>
benjiro
force-pushed
the
feat/provider-hooks
branch
from
deaaaaa
to
8cb5f6b
Compare
| @@ -209,6 +209,7 @@ public FeatureClient(IFeatureProvider featureProvider, string name, string versi | |||
| .Concat(OpenFeature.Instance.GetHooks()) | |||
| .Concat(this._hooks) | |||
| .Concat(options?.Hooks ?? Enumerable.Empty<Hook>()) | |||
| .Concat(this._featureProvider.GetProviderHooks()) | |||
There was a problem hiding this comment.
I think the merging works as expected here (basically, hooks win... see: https://github.com/open-feature/spec/blob/main/specification/sections/04-hooks.md#requirement-434)
But I don't see a test that specifically asserts this behavior (let me know if I'm wrong). If I'm correct, and it's alright with you @benjiro , I will write this test myself just to make sure I can get this project setup fully locally (I've pulled it but never run the tests suite).
EDIT:
Actually, it seems you do have a test for 4.3.4, but it needs to be updated... which I'd still like to do if you don't mind.
There was a problem hiding this comment.
Yea go ahead and update the test case. Let me know if you have any issues I've only tested the project via command line, Rider and VS. if you're using vscode you might need to fix up the git ignores or do some initial setup. If you run into any troubles ping me on slack happy to help
There was a problem hiding this comment.
I'm going to branch of this branch to make this change, feel free to merge or not.
|
@toddbaert fine with you tackling the test, did you want to merge this PR or add the test to it? |
Go ahead and merge, I'll do it in a separate PR. Will start on that today. |
Implement provider hook spec open-feature/spec#119
so default implementations can exist