-
Notifications
You must be signed in to change notification settings - Fork 21
feat: add AddHandler extension method to Dependency Injection package #462
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
feat: add AddHandler extension method to Dependency Injection package #462
Conversation
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #462 +/- ##
==========================================
+ Coverage 86.72% 86.87% +0.14%
==========================================
Files 42 43 +1
Lines 1703 1722 +19
Branches 179 180 +1
==========================================
+ Hits 1477 1496 +19
Misses 187 187
Partials 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
beeme1mr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I just had a question about the random name generator but it isn't a blocker.
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
askpt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Great work. Added a couple of suggestions 👍
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
* Add check if passed in handler name is an empty or null string Signed-off-by: Kyle Julian <[email protected]>
askpt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a very minor request! We should also validate for inputs like whitespace.
Also, @arttonoyan, would you mind double-checking?
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
test/OpenFeature.DependencyInjection.Tests/OpenFeatureBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
* Ensure that calling AddHandler multiple times will actually add all instances to the underlying OpenFeature API * Add unit tests to cover this behaviour Signed-off-by: Kyle Julian <[email protected]>
…-via-dependencyinjection Signed-off-by: Kyle Julian <[email protected]>
askpt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a minor question, and should be ready to merge! Thanks!
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenFeature.DependencyInjection/Internal/EventHandlerDelegateWrapper.cs
Outdated
Show resolved
Hide resolved
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
* Throw ArgumentException when HandlerName is null or empty * Use primary constructor in EventHandlerDelegateWrapper Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
|
LGTM! If @arttonoyan is happy with the changes @kylejuliandev made, we can merge it 👍 |
src/OpenFeature.DependencyInjection/Internal/FeatureLifecycleManager.cs
Outdated
Show resolved
Hide resolved
* Ensure when multiple handlers are registered that they are both registered * Tweak CreateServerAsync to make it easier to extend dependency injection tests Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
* Remove any unnecessary unit tests * Update integration tests to use openFeatureBuilder for easily adjusting the Arrange part of each test Signed-off-by: Kyle Julian <[email protected]>
This PR
Example usage:
Related Issues
Fixes #457
Notes
Follow-up Tasks
How to test