Skip to content
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

Empty string if out of range #65

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

droppingin
Copy link

@droppingin droppingin commented Aug 30, 2022

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!

@droppingin droppingin changed the title Feature/empty_string Empty string if out of range Sep 4, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2022

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
Copy link
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.

@droppingin droppingin force-pushed the feature/emptyString branch from 4919e1c to b954aa6 Compare April 10, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants