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.
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
Error object v2 #1371
Error object v2 #1371
Changes from 15 commits
55ccbde
393323d
18a60b8
bd0028a
0ebe9e3
668069a
2cb7b05
a645103
e579b5a
4326117
4144986
35eef34
9ce22bc
b3ebe8b
41e022e
45d780b
9a9bd2a
83a9427
9599278
02ff680
24f4ae4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Related to oddity of
ErrorScope
, I am not sure why we use untypedString
, then functional interface forErrorScope
? Why not plain old Enum value?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.
the scope list is different for each family so there is no single enum with them all. i.e. we do not use the FILTER scope with a SERVER family.
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.
Is the idea it's ok to have same "code" (like CLIENT_ERROR) for multiple scopes? I guess that is the case.
One concern is that to know which problem you have, it is no longer possible to group failures solely by Code, as that will conflate Codes with different Scopes.
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.
will add a fully qualified code to the API exception, we can then use it later for metrics etc
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.
This is not a good method name. Get what?
So suggesting we rename to something like "toApiException()" or "asApiException()"
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.
will look for better name - discussed
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.
Do we need this method? We already have overload with
Map
, why/where do we need this?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.
there to make it easy when there is only one or two variables to pass. The idea was to make it as easy as possible to instantiate exceptions