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
Implement HttpRequestError #88974
Implement HttpRequestError #88974
Changes from all commits
8cad616
511e16c
12e67e7
633d632
9afb790
c7ac73b
2eb3a4a
24dd331
b54e8c7
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.
Why is this nullable if we have Unknown, or the other way around?
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.
null
means that the underlying handler did not implement the error, whileUnknown
means generic/uncategorized error.IMHO making difference between the two is very impractical and can be a source of problems, but that's where this ended after reacting to concerns in #76644 (comment) by removing nullability and defining a default
0
error code, and then agreeing with the advice from the API review group that nullable actually makes sense.My recommendation would be to have a follow-up discussion on this and potentially remove
Unknown
in RC1.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.
I don't understand why nullable makes sense here. I was away for the API review, and see the comments in the issue:
but I agree that this is a confusing distinction to try to make. What action is someone supposed to take differently based on null vs Unknown? You get an exception, and if an error code couldn't be produced, for whatever reason, it makes sense that it's "unknown". I don't know what I'd do differently between a null error code and an Unknown error code other than have to write more code and be confused.
cc: @bartonjs, @terrajobst
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 main distinction that I can think of would be "I should try again if the error is [some specific code(s)], and not if not, and Unknown is clearly not one of those codes". If it's null (vs Unknown) then the caller can decide "the underlying provider doesn't know how to set this code" is different from "something went wrong that the caller didn't know how to map to this enum"
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.
I still don't understand the distinction / how someone would apply it. You're saying someone would write code that would retry on null but not on Unknown, or retry on Unknown but not null? Whether the provider knows what the problem is but can't map it, or doesn't know and can't map it, the result is the same: the error is unknown and you as the consumer have zero additional information to inform you as to what to do with it. I'm not understanding why we'd choose two different representations for that same case.
If there's an additional error condition that can't be mapped to one in our enum, I'd think the right answer would be to augment the enum with the additional values for those conditions.