Open
Conversation
Codecov Report
@@ Coverage Diff @@
## master #65 +/- ##
=======================================
Coverage 66.23% 66.23%
=======================================
Files 4 4
Lines 465 465
=======================================
Hits 308 308
Misses 146 146
Partials 11 11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Owner
|
All of these points are correct, but I have to confess the "Day(451)" values have always bothered me, I feel like that kinda change might break more people than we might expect. I think we might be stuck with this, I'm not saying no, just need to really think about it more. |
error will be passed through all the converters
4919e1c to
b954aa6
Compare
This file contains hidden or 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
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.
First of all, @dmarkham, thank you for the wonderful library.
We are happily use it in our go project.
I will go straight to the case.
When the receiver does not support the new enum value, it accepts it formally.
Next, the string representation returns a formatted value, like "Day(451)".
I allowed myself to return an empty string in case of errors.
I understand that it reduces verbosity but i think it matches better the go principles.
The other thing is error handling. Some converters always return nil error despite of the fact that the value is incorrect.
I have took it into account and implemented it as follows:
As you can see, the superposition of error handing e.g. in SQL marshaller and formatting of unknown value can lead to funny behavior.
In our case, there was a bug in our code related to the 2 described edge-cases.
Now i present the PR to you hoping you find some time to look at it and tell your opinion.
Thank you for your attention!