Empty string if out of range
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:
func (i Day) string() (string, error) {
// the actual implementation
}
func (i Day) String() string {
val, _ := i.string() // just Stringer, ignores error
return val
}
func (i Day) MarshalYAML() (interface{}, error) {
return i.string() // and here we use the actual implementation, that supports error return
}
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!
Codecov Report
Merging #65 (4919e1c) into master (6e1910c) will not change coverage. The diff coverage is
100.00%.
@@ Coverage Diff @@
## master #65 +/- ##
=======================================
Coverage 66.23% 66.23%
=======================================
Files 4 4
Lines 465 465
=======================================
Hits 308 308
Misses 146 146
Partials 11 11
| Impacted Files | Coverage Δ | |
|---|---|---|
| enumer.go | 100.00% <ø> (ø) |
|
| sql.go | 100.00% <ø> (ø) |
|
| stringer.go | 61.04% <100.00%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
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.