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

Unexported error sentinel values #24

Open
romshark opened this issue Sep 6, 2023 · 1 comment
Open

Unexported error sentinel values #24

romshark opened this issue Sep 6, 2023 · 1 comment

Comments

@romshark
Copy link

romshark commented Sep 6, 2023

structtag/tags.go

Lines 11 to 19 in 2977b8d

var (
errTagSyntax = errors.New("bad syntax for struct tag pair")
errTagKeySyntax = errors.New("bad syntax for struct tag key")
errTagValueSyntax = errors.New("bad syntax for struct tag value")
errKeyNotSet = errors.New("tag key does not exist")
errTagNotExist = errors.New("tag does not exist")
errTagKeyMismatch = errors.New("mismatch between key and tag.key")
)

Is there a particular reason these error sentinel values are unexported?
Because of this I have to match errors by message, which is obviously bad:

tag, err := tags.Get(expectTag)
switch {
case err.Error() == "tag does not exist":
	// handle errTagNotExist
case err != nil:
	// handle other errors
}

Instead, it should be:

tag, err := tags.Get(expectTag)
switch {
case errors.Is(err, structtag.ErrTagNotExist):
	// handle
case err != nil:
	// handle other errors
}

I propose exporting error sentinel values. Such a change would be backward compatible anyway.

@fatih
Copy link
Owner

fatih commented Sep 7, 2023

Hi @romshark

There are no particular reasons for that. The only thing was I didn't wanted to make it part of the public API, because then it would mean I wouldn't be able to make any changes. So it's an additional layer of maintenance we have to take care of. I wrote it here: #16

But given the fact this is something people rely on it, I think it makes sense to expose them.

Would you mind opening a PR? The #16 is not mergable anymore. Let's fix this.

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 a pull request may close this issue.

2 participants