Adding displaying of flags when using unknown flags for a command #239
Conversation
LMCROSSITXSADEPLOY-1100
| func collectUnknownFlags(flags *flag.FlagSet, args []string) []string { | ||
| var unknownFlags []string | ||
|
|
||
| for i := 0; i < len(args); i++ { | ||
| currentArgument := args[i] | ||
|
|
||
| if !strings.HasPrefix(currentArgument, "-") { | ||
| continue | ||
| } | ||
|
|
||
| currentFlag := currentArgument | ||
| flagName := strings.TrimLeft(currentFlag, "-") | ||
|
|
||
| if flagName == "" { | ||
| continue | ||
| } | ||
|
|
||
| isFlagKnown := flags.Lookup(flagName) | ||
| if isFlagKnown != nil { | ||
| nextIndex := i + 1 | ||
| if nextIndex < len(args) { | ||
| isBoolean := isBoolFlag(isFlagKnown) | ||
| if !isBoolean { | ||
| nextArgument := args[nextIndex] | ||
| nextHasPrefixDash := strings.HasPrefix(nextArgument, "-") | ||
| if !nextHasPrefixDash { | ||
| i = nextIndex | ||
| } | ||
| } | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| unknownFlags = append(unknownFlags, currentFlag) | ||
|
|
||
| nextIndex := i + 1 | ||
| if nextIndex < len(args) { | ||
| nextArgument := args[nextIndex] | ||
| nextHasPrefixDash := strings.HasPrefix(nextArgument, "-") | ||
| if !nextHasPrefixDash { | ||
| i = nextIndex | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return unknownFlags | ||
| } |
There was a problem hiding this comment.
This function handles a bunch of responsibilities - identifies flags, extracts their names, checks if they’re known, and collects the unknown ones. Consider breaking it down into smaller helper functions to improve readability and maintainability.
There was a problem hiding this comment.
Right, thanks for the suggestion - fixed!
commands/flags_parser.go
Outdated
| if unknownFlags := collectUnknownFlags(p.flag, args); len(unknownFlags) > 0 { | ||
| return fmt.Errorf("Unknown or wrong flags: %s", strings.Join(unknownFlags, ", ")) | ||
| } | ||
|
|
There was a problem hiding this comment.
If the same unknown flag is present more than once. How many times will it be printed here?
There was a problem hiding this comment.
Definitely more than once - there will be duplicates. Thanks for the suggestion- fixed!
…nt at the end result LMCROSSITXSADEPLOY-1100
LMCROSSITXSADEPLOY-1100
commands/flags_parser.go
Outdated
| if unknownFlags := collectUnknownFlags(p.flag, args); len(unknownFlags) > 0 { | ||
| return fmt.Errorf("Unknown or wrong flags: %s", strings.Join(unknownFlags, ", ")) | ||
| } |
There was a problem hiding this comment.
Is this check going to be executed on each parse? Why not execute it only when the parsing fails?
There was a problem hiding this comment.
yes, the check is going to be executed for each new command's arguments. I thought it was best to have this check for unknown flags as early as possible, because I think its rather a big mistake having unsupported flags and if there are any - then it is better to directly exit with an error and not even go through the execution of the internal ParseFlags() method and after it is finished to make the check for unknown flags in the block after thats supposed to take care of errors returned by ParseFlags() method.
There was a problem hiding this comment.
This approach is cleaner code-wise since its clearly separated and fails earlier. Still, if we assume generally that errors with flags are more rare, I think that a check only when the flags.Parse() fails would be preferred.
Also there is the added risk of false-positives when checking before flags.Parse(), since were essentially adding more restrictions to the command execution with the new logic.
LMCROSSITXSADEPLOY-1100
3 participants
LMCROSSITXSADEPLOY-1100