-
-
Notifications
You must be signed in to change notification settings - Fork 133
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
Refactor overflowError to be pretty-printable #470
Refactor overflowError to be pretty-printable #470
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #470 +/- ##
==========================================
- Coverage 78.77% 78.76% -0.01%
==========================================
Files 13 13
Lines 4004 4003 -1
==========================================
- Hits 3154 3153 -1
Misses 591 591
Partials 259 259 |
decode.go
Outdated
} | ||
return errOverflow(valueType, fmt.Sprint(v)) | ||
return errTypeMismatch(valueType, reflect.TypeOf(v), src.GetToken()) |
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 need the overflow information, so could you modify the errOverflow
function to pass src.GetToken
?
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.
Thanks for your review. I am going to modify the error as you suggested.
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 have changed the approach of this PR. Instead of using another error, I have refactored the overflowError
to be a PrettyPrinter
and Formatter
like the other error types.
Please take another look and thanks again for your review and providing this library.
When trying to yaml.Unmarshal a negative number into an uint, the resulting error indicates an overflow with type information, but without the position. For example: > cannot unmarshal -23 into Go value of type uint64 ( overflow ) From an end user's perspective reading an error for an invalid YAML, this is quite unintuitive. Especially compared to the error message when trying to unmarshal a string into an uint. > [1:4] cannot unmarshal string into Go struct field Foo.A of type uint64 > > 1 | a: 'foo' > ^ This change has moved overflowError to internal.errors.overflowError, implementing both the PrettyPrinter and Formatter. Its implementation is uniform with those of the syntaxError and TypeError. Thus, the error from above now looks like: > [1:4] cannot unmarshal -23 into Go value of type uint64 ( overflow ) > > 1 | a: -23 > ^
525e201
to
8bfb9ef
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.
Thank you for your contribution ! LGTM 👍
When trying to yaml.Unmarshal a negative number into an uint, the resulting error indicates an overflow with type information, but without the position. For example:
From an end user's perspective reading an error for an invalid YAML, this is quite unintuitive. Especially compared to the error message when trying to unmarshal a string into an uint.
This change has moved overflowError to internal.errors.overflowError, implementing both the PrettyPrinter and Formatter. Its implementation is uniform with those of the syntaxError and TypeError.
Thus, the error from above now looks like: