feat!: ErrorType as enum, add ErrorMessage string #72
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #72 +/- ##
==========================================
- Coverage 94.44% 94.13% -0.32%
==========================================
Files 18 18
Lines 432 426 -6
Branches 36 36
==========================================
- Hits 408 401 -7
Misses 13 13
- Partials 11 12 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
kinyoklion
force-pushed
the
feat/error-handling-0.5.0-spec
branch
from
7a64766
to
c480ecb
Compare
kinyoklion
commented
Sep 27, 2022
| @@ -1,5 +1,4 @@ | |||
| using OpenFeatureSDK.Constant; | |||
There was a problem hiding this comment.
This file was renamed Evalusation -> Evaluation
kinyoklion
commented
Sep 27, 2022
| [Specification("2.5", "In cases of normal execution, the `provider` SHOULD populate the `flag resolution` structure's `variant` field with a string identifier corresponding to the returned flag value.")] | ||
| [Specification("2.6", "The `provider` SHOULD populate the `flag resolution` structure's `reason` field with a string indicating the semantic reason for the returned flag value.")] | ||
| [Specification("2.7", "In cases of normal execution, the `provider` MUST NOT populate the `flag resolution` structure's `error code` field, or otherwise must populate it with a null or falsy value.")] | ||
| [Specification("2.9", "In cases of normal execution, the `provider` MUST NOT populate the `flag resolution` structure's `error code` field, or otherwise must populate it with a null or falsy value.")] |
There was a problem hiding this comment.
2.9 was a duplicate of 2.7. I've updated it to 2.9.1 and transcribed the text. This test already demonstrated the associated specification supporting generics.
kinyoklion
commented
Sep 27, 2022
| @@ -321,6 +321,36 @@ public async Task Should_Resolve_StructureValue() | |||
| featureProviderMock.Verify(x => x.ResolveStructureValue(flagName, defaultValue, It.IsAny<EvaluationContext>()), Times.Once); | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
I added an extra test here for the case where exceptions are not being used.
Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
benjiro
approved these changes
Sep 28, 2022
benjiro
reviewed
Sep 28, 2022
| @@ -1,5 +1,4 @@ | |||
| using OpenFeatureSDK.Constant; | |||
toddbaert
reviewed
Sep 28, 2022
Comment on lines
23
to
25
| public FeatureProviderException(ErrorType errorType, string message = null, Exception innerException = null) | ||
| : base(message, innerException) | ||
| { |
There was a problem hiding this comment.
In Java we use dedicated exception types for each error code. I'm not saying that's better, just an option to consider.
The current implementation here might actually be easier for some providers 🤷
toddbaert
reviewed
Sep 28, 2022
toddbaert
approved these changes
Sep 28, 2022
4 tasks
This was referenced Jan 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses: #71
Extend the
ErrorTypeto includeINVALID_CONTEXTandTARGETING_KEY_MISSING.Change the
ErrorTypeinFlagEvaluationDetailsto an enum.Update the
FeatureProviderExceptionto have an enumerated code. Usemessagefor theerror message.Update tests, and update specification references in tests.