feat: Handling of incorrectly used flag #3901
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes refactor the Terragrunt CLI’s initialization and flag error handling. The modifications introduce a modular approach to command management via a new commands variable and add a dedicated flag error handler to the App. Error messages for undefined flags have been simplified both in tests and during command execution. Additionally, the flag package now supports retrieving deprecated flag names, and a new function type standardizes error handler functions for flag parsing. These updates streamline error reporting and improve clarity across the CLI’s internal flows. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as App
participant C as Command
participant F as FlagErrHandler
U->>A: Launch CLI with options
A->>C: Initialize commands (terragruntCommands)
U->>C: Execute command with flags
C->>C: Parse flags
alt Error encountered during flag parsing
C-->>F: Report undefined flag error
F-->>C: Return formatted error with hint
else Successful parsing
C-->>A: Continue execution
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
internal/cli/app.go
Outdated
| @@ -70,6 +73,8 @@ type App struct { | |||
| // library. This library supports bash, zsh and fish. To add support | |||
| // for other shells, please see that library. | |||
| AutocompleteInstaller AutocompleteInstaller | |||
|
|
|||
| // HandleFlagError | |||
There was a problem hiding this comment.
Forgot to remove, thanks!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/cli/funcs.go (1)
25-26: Add an example to the documentation.The new
FlagErrHandlerFuncwould benefit from an example in the documentation, similar to howHelpFuncis documented. This would help other developers understand its usage better.Add this documentation:
// FlagErrHandlerFunc is executed if an error occurs while parsing flags. +// Example: +// +// func handleFlagError(ctx *Context, err error) error { +// if err != nil { +// fmt.Printf("Flag error: %v\n", err) +// } +// return err +// } type FlagErrHandlerFunc func(ctx *Context, err error) errorcli/flags/error_handler.go (1)
24-40: Optimize the nested loops with early returns.The nested loops could be refactored for better readability and performance. Also, consider extracting the flag name check into a separate function.
Here's a suggested refactor:
- for _, cmd := range commands { - for _, flag := range cmd.Flags { - flagNames := flag.Names() - - if flag, ok := flag.(interface{ DeprecatedNames() []string }); ok { - flagNames = append(flagNames, flag.DeprecatedNames()...) - } - - if !slices.Contains(flagNames, undefFlag) { - continue - } - - flagName := util.FirstElement(util.RemoveEmptyElements(flag.Names())) - - return errors.Errorf(flagHintFmt, undefFlag, ctx.Command.Name, cmd.Name, flagName) - } - } + if cmd, flag := findFlagInCommands(commands, undefFlag); cmd != nil { + flagName := util.FirstElement(util.RemoveEmptyElements(flag.Names())) + return errors.Errorf(flagHintFmt, undefFlag, ctx.Command.Name, cmd.Name, flagName) + }Add this helper function:
func findFlagInCommands(commands cli.Commands, undefFlag string) (*cli.Command, cli.Flag) { for _, cmd := range commands { for _, flag := range cmd.Flags { flagNames := flag.Names() if flag, ok := flag.(interface{ DeprecatedNames() []string }); ok { flagNames = append(flagNames, flag.DeprecatedNames()...) } if slices.Contains(flagNames, undefFlag) { return cmd, flag } } } return nil, nil }cli/flags/flag.go (1)
54-66: Optimize slice allocation and add nil checks.The slice handling could be more efficient, and we should add nil checks for robustness.
Here's a suggested improvement:
func (newFlag *Flag) DeprecatedNames() []string { - var names []string + if newFlag == nil { + return nil + } + + // Pre-allocate slice with estimated capacity + names := make([]string, 0, len(newFlag.deprecatedFlags)*2) if flag, ok := newFlag.Flag.(interface{ DeprecatedNames() []string }); ok { - names = flag.DeprecatedNames() + names = append(names, flag.DeprecatedNames()...) } for _, deprecated := range newFlag.deprecatedFlags { + if deprecated == nil { + continue + } names = append(names, deprecated.Names()...) } return names }internal/cli/command.go (1)
77-78: Remove or complete the incomplete comment.The comment "HandleFlagError" appears to be incomplete. Either complete the documentation or remove it to maintain code cleanliness.
- // HandleFlagError
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cli/app.go(3 hunks)cli/app_test.go(1 hunks)cli/flags/error_handler.go(1 hunks)cli/flags/flag.go(1 hunks)internal/cli/app.go(2 hunks)internal/cli/command.go(2 hunks)internal/cli/errors.go(1 hunks)internal/cli/funcs.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
🔇 Additional comments (6)
internal/cli/errors.go (1)
110-116: Clean and clear error handling!The error message format is consistent and user-friendly. Good job on making the undefined flag errors more descriptive.
internal/cli/command.go (2)
103-108: Nice improvement to error handling!The new error handling block elegantly delegates flag errors to a dedicated handler while maintaining backward compatibility. This separation of concerns makes the code more maintainable and allows for customized flag error handling.
229-229: Clean error handling refactor!The simplified error creation using
UndefinedFlagErrormakes the code more readable and maintainable by removing unnecessary string manipulation.internal/cli/app.go (1)
52-54: Well-documented new field addition!The new
FlagErrHandlerfield is clearly documented and follows good naming conventions. It enhances the App's error handling capabilities by providing a dedicated handler for flag parsing errors.cli/app.go (1)
53-54: Nice refactoring for better modularity!The changes improve code organization by:
- Extracting command creation into a variable for better reusability
- Using the extracted commands consistently
- Properly initializing the new error handler
Also applies to: 63-64, 67-68
cli/app_test.go (1)
64-65: Tests properly updated to match new behavior!The test cases have been correctly updated to verify the new simplified error messages for undefined flags. This ensures the new error handling behavior is properly tested.
Also applies to: 70-71
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cli/flags/error_handler.go (2)
12-38: Solid error handler implementation with room for a small improvementThe error handler is well-structured and provides good suggestions. However, we could make the error type assertion more explicit for better maintainability.
Consider this small improvement:
- var undefinedFlagErr cli.UndefinedFlagError - if !errors.As(err, &undefinedFlagErr) { + undefinedFlagErr, isUndefinedFlag := err.(cli.UndefinedFlagError) + if !isUndefinedFlag {
40-62: Consider optimizing the flag search for large command treesThe recursive flag search is well-implemented but might benefit from early returns in deeply nested command trees.
Consider adding an early return when all subcommands are searched:
func findFlagInCommands(commands cli.Commands, undefFlag string) (cli.Commands, cli.Flag) { + if len(commands) == 0 { + return nil, nil + }internal/cli/commands.go (1)
22-31: Simple and effective command names collectionThe
Names()method is clean and straightforward. For better memory efficiency in larger command sets, consider pre-allocating with a capacity hint.Here's a small optimization:
- var names = make([]string, len(commands)) + names := make([]string, 0, len(commands)) for i, cmd := range commands { - names[i] = cmd.Name + names = append(names, cmd.Name)test/integration_test.go (1)
121-151: Solid test implementation with room for enhancement!The test function is well-structured using table-driven tests and proper cleanup. Consider these improvements:
- Add more descriptive test case names instead of using array indices
- Include additional test cases for edge scenarios like:
- Multiple incorrect flags
- Empty flag values
- Invalid flag formats
Here's how you could improve the test cases:
testCases := []struct { + name string args string expectedError error }{ { + name: "invalid global flag raw used with init", args: "-raw init", expectedError: flags.NewGlobalFlagHintError("raw", "stack output", "raw"), }, { + name: "invalid command flag no-include-root used with run", args: "run --no-include-root", expectedError: flags.NewCommandFlagHintError("run", "no-include-root", "catalog", "no-include-root"), }, + { + name: "multiple invalid flags", + args: "run --no-include-root -raw", + expectedError: flags.NewCommandFlagHintError("run", "no-include-root", "catalog", "no-include-root"), + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cli/flags/error_handler.go(1 hunks)cli/flags/errors.go(1 hunks)cli/flags/flag.go(1 hunks)internal/cli/app.go(1 hunks)internal/cli/commands.go(1 hunks)test/fixtures/cli-flag-hints/main.hcl(1 hunks)test/fixtures/cli-flag-hints/terragrunt.hcl(1 hunks)test/integration_test.go(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/fixtures/cli-flag-hints/terragrunt.hcl
- test/fixtures/cli-flag-hints/main.hcl
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/flags/flag.go
- internal/cli/app.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (4)
cli/flags/errors.go (2)
5-23: Nice implementation of the global flag error handling! 👍The
GlobalFlagHintErrorimplementation is clean and user-friendly. The error message format "Did you mean to use..." is particularly helpful for users.
25-45: Well-structured command flag error handling! 🎯The
CommandFlagHintErrorfollows the same clean pattern as the global flag error handler. Both error types together provide a consistent experience for users when they misuse flags.test/integration_test.go (2)
20-20: Clean import addition!The new flags package import is well-placed and properly organized.
111-111: Well-structured constant addition!The new test fixture constant follows the established naming pattern and is properly ordered.
|
@yhakbar Thanks! |
Description
Fixes #3836.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes