-
Notifications
You must be signed in to change notification settings - Fork 356
fixes error handling to match specification #727
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
8f5f49b
to
e11f307
Compare
Signed-off-by: Oleh Dokuka <[email protected]>
e11f307
to
0322be9
Compare
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.
“|| errorCode <= MAX_USER_ALLOWED_ERROR_CODE) {“
This appears broken due to overflow
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.
if (errorCode > ErrorType.MAX_USER_ALLOWED_ERROR_CODE
+ && errorCode < ErrorType.MIN_USER_ALLOWED_ERROR_CODE)
Looks iffy. Should we perform these checks in unsigned longs?
return new InvalidException(message); | ||
default: | ||
if (errorCode >= MIN_USER_ALLOWED_ERROR_CODE | ||
|| errorCode <= MAX_USER_ALLOWED_ERROR_CODE) { |
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.
On re-reading on a computer (not Github iOS tesatflight app :), this seems good.
if (errorCode >= 769
|| errorCode <= -2) {
if (errorCode > ErrorType.MAX_USER_ALLOWED_ERROR_CODE | ||
&& errorCode < ErrorType.MIN_USER_ALLOWED_ERROR_CODE) { |
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 legit, sorry.
if (errorCode > -2
&& errorCode < 769) {
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
So, yeah, to clarify, since error API expect int and return int, we cannot use long for that purpose, that is why had to deal with such unobvious comparison |
Signed-off-by: Oleh Dokuka <[email protected]>
This PR extends the current implementation of error handling to match what is defined in the spec and let the user use a range of user allowed error codes using
CastomRSocketException
classSigned-off-by: Oleh Dokuka [email protected]