Prevent pushing with empty Label/Tag names #2780
Conversation
| for _, tag := range flags.Tags { | ||
| if tag == "" { | ||
| return appcmd.NewInvalidArgumentErrorf( | ||
| "%s|--%s requires a non-empty string.", |
There was a problem hiding this comment.
drive by, but should the short flag have a - in front?
| "%s|--%s requires a non-empty string.", | |
| "-%s|--%s requires a non-empty string.", |
There was a problem hiding this comment.
Ah I x'd that by accident. Let me push that up.
| )..., | ||
| ), | ||
| ) | ||
| labels := make([]string, 0, len(flags.Labels)+len(flags.Tags)) |
There was a problem hiding this comment.
Why not continue to use previous style, labels := append(flag.Labels, flags.Tags...)?
There was a problem hiding this comment.
append can mutate the underlying array of its first argument (flag.Label). It shouldn't matter in this case because we're not overwriting indexes, but in general I avoid using append to copy a slice I don't own. Happy to change this back if you think it's a non-issue.
There was a problem hiding this comment.
Well every day's a school day I guess, I don't know how I've gone this long without running into some bug based on this. Sounds good. A slight nit, I might clean it up to append(slicesext.Copy(flags.Labels), flags.Tags...) but doesn't matter much.
There was a problem hiding this comment.
I cleaned it up using slicesext.Copy
There was a problem hiding this comment.
@akshayjshah on my case about how could I be so naive
| for _, tag := range flags.Tags { | ||
| if tag == "" { | ||
| return appcmd.NewInvalidArgumentErrorf( | ||
| "-%s|--%s requires a non-empty string.", |
There was a problem hiding this comment.
All other usages of appcmd.NewInvalidArgumentErrorf (save one) in the codebase do not display short names, and do not use punctuation - update to --%s requires a non-empty string
There was a problem hiding this comment.
Did you want me to remove the trailing period on the errors returned by validateCreateFlags?
There was a problem hiding this comment.
Yea for sure, that was a miss on my part
| return nil | ||
| } | ||
|
|
||
| func validateLabelFlags(flags *flags) error { |
There was a problem hiding this comment.
Put below validateCreateFlags since this is the call order and it is more natural
| func validateLabelFlags(flags *flags) error { | ||
| for _, label := range flags.Labels { | ||
| if label == "" { | ||
| return appcmd.NewInvalidArgumentErrorf( |
| } | ||
| for _, tag := range flags.Tags { | ||
| if tag == "" { | ||
| return appcmd.NewInvalidArgumentErrorf( |
flags.Draftandflags.Branchmay be empty string, and they were being added to the labels name. Instead, I conditionally add them.I also chose to detect empty strings in
flags.Labelsandflags.Tags, via--label ''or--tag ''.