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.

Sign up for GitHub

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

Fix missing required flag error uses flag name and not alias #1709

Merged
merged 2 commits into from
Mar 28, 2023
Merged

Fix missing required flag error uses flag name and not alias #1709

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7e3459
fix: missing required flag error uses flag name and not alias
nirhaas Mar 20, 2023
69df7bf
add test case for flag without name or aliases
nirhaas Mar 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,10 @@ func (cCtx *Context) checkRequiredFlags(flags []Flag) requiredFlagsErr {
var flagPresent bool
var flagName string

for _, key := range f.Names() {
flagName = key
flagNames := f.Names()
flagName = flagNames[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if someone passes in a flag with no name or aliases defined ? Will the slice be nil ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 🦾

Copy link
Author

@nirhaas nirhaas Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It returns an empty slice (length 0) slice with empty string (length 1), not a nil. I can make a test case for it to prove that it doesn't panic if you want, even though the code doesn't handle this kind of flag nicely (showing as a newline in the help).

What would a flag without a name or alias means to the user? Shouldn't the code have a different check and error for this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if len(flagNames) == 0 ln 208 will panic:

https://go.dev/play/p/aN-gKini_p9

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skelouse it's length 1 with an empty string. Sorry for the confusion. https://go.dev/play/p/tsnBea9RKk4 - No panic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both examples provided are illustrative but its better to add a simple test case for this. Just have a flag without a name and see what happens. If everything works well and good.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Added another test case


for _, key := range flagNames {
if cCtx.IsSet(strings.TrimSpace(key)) {
flagPresent = true
}
Expand Down
8 changes: 8 additions & 0 deletions context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,14 @@ func TestCheckRequiredFlags(t *testing.T) {
&StringFlag{Name: "n", Required: true},
},
},
{
testCase: "required_flag_with_alias_errors_with_actual_name",
expectedAnError: true,
expectedErrorContents: []string{"Required flag \"collection\" not set"},
flags: []Flag{
&StringFlag{Name: "collection", Required: true, Aliases: []string{"c"}},
},
},
}

for _, test := range tdata {
Expand Down