-
Notifications
You must be signed in to change notification settings - Fork 22
fix: Refactor error handling and improve documentation #417
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
Conversation
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…lag and improve assertions Signed-off-by: André Silva <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #417 +/- ##
==========================================
- Coverage 85.71% 85.69% -0.02%
==========================================
Files 39 39
Lines 1603 1601 -2
Branches 171 171
==========================================
- Hits 1374 1372 -2
Misses 191 191
Partials 38 38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
test/OpenFeature.Tests/OpenFeatureClientTests.cs:187
- [nitpick] Consider adding a comment in the test to clarify that error logging is intentionally disabled during flag evaluation to help future maintainers understand this behavior.
mockedLogger.Received(0).IsEnabled(LogLevel.Error);
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
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.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/OpenFeature/Providers/Memory/InMemoryProvider.cs:115
- [nitpick] Consider returning a ResolutionDetails with an appropriate error type for type mismatches instead of throwing an exception, to align the error handling behavior with that of missing flags. If the current behavior is by design, please add documentation explaining the rationale.
throw new TypeMismatchException("flag " + flagKey + " is not of type " + typeof(T));
Signed-off-by: André Silva <[email protected]>
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.
changes look good to me
I agree with the change on the flag-not-found. I prefer not to use exceptions to control logic flow; that's an old anti-pattern, and those stack traces have a hidden cost.
Thanks!
🤖 I have created a release *beep* *boop* --- ## [2.4.0](v2.3.2...v2.4.0) (2025-04-14) ### 🐛 Bug Fixes * Refactor error handling and improve documentation ([#417](#417)) ([b0b168f](b0b168f)) ### ✨ New Features * update FeatureLifecycleStateOptions.StopState default to Stopped ([#414](#414)) ([6c23f21](6c23f21)) ### 🧹 Chore * **deps:** update github/codeql-action digest to 45775bd ([#419](#419)) ([2bed467](2bed467)) * restrict publish to environment ([#431](#431)) ([0c222cb](0c222cb)) ### 📚 Documentation * Update contributing guidelines ([#413](#413)) ([84ea288](84ea288)) ### 🔄 Refactoring * simplify the InternalsVisibleTo usage ([#408](#408)) ([4043d3d](4043d3d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <[email protected]>
) Signed-off-by: André Silva <[email protected]> <!-- Please use this template for your pull request. --> <!-- Please use the sections that you need and delete other sections --> ## This PR <!-- add the description of the PR here --> This pull request includes updates to the `README.md`, changes to error handling in `OpenFeatureClient`, and improvements to the `InMemoryProvider` and its related tests. Documentation updates: * Added notes and examples to the `README.md` to improve clarity on logging, transaction context propagators, dependency injection, and custom providers. [[1]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R4) [[2]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R20) [[3]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R157) [[4]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R170) [[5]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R266) [[6]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R276) [[7]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R360-R378) [[8]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R387-R390) [[9]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R406) [[10]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R450) [[11]](diffhunk://#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R469) Error handling improvements: * Removed redundant error logging methods `FlagEvaluationError` and `FlagEvaluationErrorWithDescription` in `OpenFeatureClient`. [[1]](diffhunk://#diff-c23c8a3ea4538fbdcf6b1cf93ea3de456906e4d267fc4b2ba3f8b1cb186a7907L279) [[2]](diffhunk://#diff-c23c8a3ea4538fbdcf6b1cf93ea3de456906e4d267fc4b2ba3f8b1cb186a7907L293) [[3]](diffhunk://#diff-c23c8a3ea4538fbdcf6b1cf93ea3de456906e4d267fc4b2ba3f8b1cb186a7907L400-L402) InMemoryProvider enhancements: * Changed `InMemoryProvider` to return `ResolutionDetails` with appropriate error types instead of throwing exceptions for missing flags or type mismatches. [[1]](diffhunk://#diff-4734ad108181b3c0b9f2c89e921b023e0d3e06d3c26ba1bed6352a75643469b0L106-R105) [[2]](diffhunk://#diff-4734ad108181b3c0b9f2c89e921b023e0d3e06d3c26ba1bed6352a75643469b0L116-R115) Test updates: * Updated tests in `OpenFeatureClientTests` and `InMemoryProviderTests` to reflect changes in error handling and ensure proper error types and reasons are returned. [[1]](diffhunk://#diff-97c93c206b866c1293c8c476783ad62d653cbac48716afc15d418b7701431e30L187-R187) [[2]](diffhunk://#diff-9cc800e07a869b42598d8f63786c01c406128056e28c995e73b13f229d9c912fL178-R185) [[3]](diffhunk://#diff-9cc800e07a869b42598d8f63786c01c406128056e28c995e73b13f229d9c912fL233-R242) Configuration changes: * Modified `global.json` to update the SDK roll-forward policy from `latestMajor` to `latestFeature`. ### Related Issues <!-- add here the GitHub issue that this PR resolves if applicable --> Fixes open-feature#416 ### Notes After evaluating the `global.json`, I noticed the [rollForward](https://learn.microsoft.com/en-us/dotnet/core/tools/global-json#rollforward) was way too high for what we should be using. I reduced to `latestFeature` which is safer and would allow us to change the GitHub Actions to use the version from `global.json` instead of from declaring it in the action itself. --------- Signed-off-by: André Silva <[email protected]> Signed-off-by: Weihan Li <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [2.4.0](open-feature/dotnet-sdk@v2.3.2...v2.4.0) (2025-04-14) ### 🐛 Bug Fixes * Refactor error handling and improve documentation ([open-feature#417](open-feature#417)) ([b0b168f](open-feature@b0b168f)) ### ✨ New Features * update FeatureLifecycleStateOptions.StopState default to Stopped ([open-feature#414](open-feature#414)) ([6c23f21](open-feature@6c23f21)) ### 🧹 Chore * **deps:** update github/codeql-action digest to 45775bd ([open-feature#419](open-feature#419)) ([2bed467](open-feature@2bed467)) * restrict publish to environment ([open-feature#431](open-feature#431)) ([0c222cb](open-feature@0c222cb)) ### 📚 Documentation * Update contributing guidelines ([open-feature#413](open-feature#413)) ([84ea288](open-feature@84ea288)) ### 🔄 Refactoring * simplify the InternalsVisibleTo usage ([open-feature#408](open-feature#408)) ([4043d3d](open-feature@4043d3d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <[email protected]> Signed-off-by: Weihan Li <[email protected]>
Signed-off-by: André Silva [email protected]
This PR
This pull request includes updates to the
README.md
, changes to error handling inOpenFeatureClient
, and improvements to theInMemoryProvider
and its related tests.Documentation updates:
README.md
to improve clarity on logging, transaction context propagators, dependency injection, and custom providers. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]Error handling improvements:
FlagEvaluationError
andFlagEvaluationErrorWithDescription
inOpenFeatureClient
. [1] [2] [3]InMemoryProvider enhancements:
InMemoryProvider
to returnResolutionDetails
with appropriate error types instead of throwing exceptions for missing flags or type mismatches. [1] [2]Test updates:
OpenFeatureClientTests
andInMemoryProviderTests
to reflect changes in error handling and ensure proper error types and reasons are returned. [1] [2] [3]Configuration changes:
global.json
to update the SDK roll-forward policy fromlatestMajor
tolatestFeature
.Related Issues
Fixes #416
Notes
After evaluating the
global.json
, I noticed the rollForward was way too high for what we should be using. I reduced tolatestFeature
which is safer and would allow us to change the GitHub Actions to use the version fromglobal.json
instead of from declaring it in the action itself.