-
Notifications
You must be signed in to change notification settings - Fork 44
Description
Hi there! I tried using the library but got a little confused.
Particularly the current documentation and API of .Get(...)
was a matter of trial & error.
// Get returns the tag associated with the given key. If the key is present
// in the tag the value (which may be empty) is returned. Otherwise, the
// returned value will be the empty string. The ok return value reports whether
// the tag exists or not (which the return value is nil).
func (t *Tags) Get(key string) (*Tag, error) {
for _, tag := range t.tags {
if tag.Key == key {
return tag, nil
}
}
return nil, errTagNotExist
}
The comment suggests a pattern like:
docTag, ok := tags.Get("documentation)
if ok {
...
}
In reality, ok
is an error, not a boolean, so it wouldn't work.
The main README adds to the confusion:
// ... and start using structtag by parsing the tag
jsonTag, err := tags.Get("json")
if err != nil {
panic(err)
}
Based on that panic I wouldn't expect the only error returned by Get
is errTagNotExist
(which perfectly OK in my scenario).
The panic example in the docs signals something more severe is going on. Moreover, because errTagNotExist
is not exported, callers can’t do something like
if errors.Is(err, structtag.ErrTagNotExist) { … }
A more sensible signature would be:
docTag, ok := tags.Get("documentation")
if ok {
// …
}
so that the caller can check existence with a boolean rather than treating “not found” as an error. Similar to Go's type checking. Unfortunately, changing Get
from (*Tag, error)
to (*Tag, bool)
would break backwards compatibility.
As an alternative, the library could introduce something like:
func (t *Tags) Find(key string) (*Tag, bool)
that returns a boolean found flag instead of an error. This would leave Get unchanged (preserving compatibility) while providing a clearer, more “Go-idiomatic” lookup method.
There's various possibilities here, not sure which is the best one. I figured I'd just share my first experience with this lib :-)