dmarkham / enumer

A Go tool to auto generate methods for your enums
Other
410 stars 66 forks source link

Empty string if out of range #65

Open droppingin opened 1 year ago

droppingin commented 1 year ago

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 in 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-commenter commented 1 year ago

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.

dmarkham commented 1 year ago

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.