feat: FlagSet: add MustSetAnnotation method#458
feat: FlagSet: add MustSetAnnotation method#458thaJeztah wants to merge 1 commit intospf13:masterfrom
Conversation
The SetAnnotation method returns an error if the given flag does not exist. While handling the error should normally be the recommendation, in many cases the list of flags is static, and adding error handling for each annotation can either introduce unwanted boilerplating, or may not be possible in cases where there's no error return. Unfortunately, also makes this method prone to copy/paste mistakes where either the flag doesn't exist, or an annotation is already set, potentially overwriting another flag's annotation (which is a bug I recently discovered in one of our projects (see [1])). Such bugs can be hard to discover if they have no immediate impact. This patch introduces a `MustSetAnnotation` that can be used as a replacement for `SetAnnotation`. Unlike `SetAnnotation`, it produces a panic instead of returning an error, and uses stricter validation; not allowing an annotation to be set if the flag already has the annotation set. [1]: https://github.com/docker/cli/blob/93fa57bbcd08f2f5be7f6cf22f4273a2b5a49e71/cli/command/service/opts.go#L905-L910 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
add_MustSetAnnotation
branch
from
06e04a2 to
918e819
Compare
tomasaschan
left a comment
tomasaschan
left a comment
There was a problem hiding this comment.
I don't want to argue against the usefulness of erroring if an annotation is already set, but I do think that this PR is a problem in terms of naming and user expectations on function pairs like func Foo() error/func MustFoo(). It is a strong convention that the MustFoo variant is basically equivalent to
func MustFoo() {
if err := Foo(); err != nil {
panic(err)
}
}
to ensure that behavior is consistent except in terms of error handling.
I propose either splitting out the duplicate detection functionality (it can be added to SetAnnotation in a backwards compatible way using functional options!) or choosing a different name for this variant (maybe something like SetAnnotationIfUnset?).
PS: Sorry this took so long to come back to. I haven't sat at a computer since before Christmas, and some bug in the phone app left me with a review draft that I couldn't find but also couldn't ignore to post a new review. Hopefully this will be less of an issue going forward, as I am going back to work after nine months of parental leave over the next couple of weeks.
| if _, ok := flag.Annotations[key]; ok { | ||
| panic(fmt.Errorf("annotation %q is already set for flag %q", key, name)) | ||
| } |
There was a problem hiding this comment.
To be explicit: this behavior differs between SetAnnotation and MustSetAnnotation, and seems like something that would both be valuable to expose without panicking, and like something that would be confusing for users of SetAnnotation who don't want to bother with checking the returned error.
2 participants
The SetAnnotation method returns an error if the given flag does not exist. While handling the error should normally be the recommendation, in many cases the list of flags is static, and adding error handling for each annotation can either introduce unwanted boilerplating, or may not be possible in cases where there's no error return.
Unfortunately, also makes this method prone to copy/paste mistakes where either the flag doesn't exist, or an annotation is already set, potentially overwriting another flag's annotation (which is a bug I recently discovered in one of our projects (see 1)). Such bugs can be hard to discover if they have no immediate impact.
This patch introduces a
MustSetAnnotationthat can be used as a replacement forSetAnnotation. UnlikeSetAnnotation, it produces a panic instead of returning an error, and uses stricter validation; not allowing an annotation to be set if the flag already has the annotation set.