chore: introduce flags helper for consistency #533
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new internal Changes
Sequence Diagram(s)sequenceDiagram
participant CLI_Command
participant flags_pkg as flags package
participant Cobra
CLI_Command->>flags_pkg: SetFlagRequired(cmd, flag, location, isPersistent)
flags_pkg->>Cobra: cmd.MarkFlagRequired(flag) (or Persistent)
Cobra-->>flags_pkg: success/error
flags_pkg-->>CLI_Command: error (if any)
CLI_Command->>CLI_Command: Print error and exit if needed
sequenceDiagram
participant CLI_Command
participant flags_pkg as flags package
participant Cobra
CLI_Command->>flags_pkg: SetFlagsRequired(cmd, [flags...], location, isPersistent)
loop for each flag
flags_pkg->>Cobra: cmd.MarkFlagRequired(flag) (or Persistent)
Cobra-->>flags_pkg: success/error
end
flags_pkg-->>CLI_Command: Combined error (if any)
CLI_Command->>CLI_Command: Print error and exit if needed
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/model/test.go (1)
103-106: Fix inconsistent location parameter.The location parameter should match the actual file path for consistency with other command files.
Apply this fix:
- if err := flags.SetFlagRequired(testCmd, "tests", "cmd/models/test", false); err != nil { + if err := flags.SetFlagRequired(testCmd, "tests", "cmd/model/test", false); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/model/get.go(2 hunks)cmd/model/list.go(2 hunks)cmd/model/test.go(2 hunks)cmd/model/write.go(2 hunks)cmd/query/list-users.go(2 hunks)cmd/store/delete.go(2 hunks)cmd/store/export.go(2 hunks)cmd/store/get.go(2 hunks)cmd/store/import.go(2 hunks)internal/flags/flags.go(1 hunks)internal/flags/flags_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
cmd/model/test.go (1)
internal/flags/flags.go (1)
SetFlagRequired(17-29)
cmd/model/write.go (1)
internal/flags/flags.go (1)
SetFlagRequired(17-29)
cmd/store/get.go (1)
internal/flags/flags.go (1)
SetFlagRequired(17-29)
cmd/store/delete.go (1)
internal/flags/flags.go (1)
SetFlagRequired(17-29)
cmd/store/import.go (1)
internal/flags/flags.go (1)
SetFlagRequired(17-29)
cmd/model/get.go (1)
internal/flags/flags.go (1)
SetFlagRequired(17-29)
cmd/query/list-users.go (1)
internal/flags/flags.go (1)
SetFlagsRequired(34-44)
cmd/store/export.go (1)
internal/flags/flags.go (1)
SetFlagRequired(17-29)
cmd/model/list.go (1)
internal/flags/flags.go (1)
SetFlagRequired(17-29)
internal/flags/flags_test.go (2)
internal/flags/flags.go (2)
SetFlagRequired(17-29)SetFlagsRequired(34-44)internal/slices/contains.go (1)
Contains(22-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Release Process
- GitHub Check: Tests
- GitHub Check: Lints
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (go)
🔇 Additional comments (22)
internal/flags/flags.go (3)
1-11: LGTM! Clean package structure.The package documentation, imports, and constants are well-organized and follow Go best practices.
17-29: LGTM! Solid implementation of single flag requirement.The function correctly handles both persistent and non-persistent flags with proper error wrapping and contextual information.
34-43: Fix potential nil error entries in error slice.The
SetFlagsRequiredfunction pre-allocates an error slice with the same length as the flags slice, but only populates entries when errors occur. This meanserrors.Join()will receive nil entries, which could lead to unexpected behavior.Apply this fix to only include actual errors:
func SetFlagsRequired(cmd *cobra.Command, flags []string, location string, isPersistent bool) error { - flagErrors := make([]error, len(flags)) + var flagErrors []error for i, flag := range flags { if err := SetFlagRequired(cmd, flag, location, isPersistent); err != nil { - flagErrors[i] = err + flagErrors = append(flagErrors, err) } } return errors.Join(flagErrors...) }Likely an incorrect or invalid review comment.
cmd/store/export.go (2)
33-33: LGTM! Proper import addition.The flags package import is correctly added and aligns with the refactoring objective.
197-200: LGTM! Correct usage of the flags helper.The replacement of direct
MarkFlagRequiredcall withflags.SetFlagRequiredis implemented correctly with proper parameters and consistent error handling.cmd/store/get.go (2)
29-29: LGTM! Proper import addition.The flags package import is correctly added and follows the established pattern.
69-72: LGTM! Correct implementation of the flags helper.The replacement correctly uses
flags.SetFlagRequiredwith appropriate parameters and maintains consistent error handling.cmd/model/test.go (1)
27-27: LGTM! Proper import addition.The flags package import is correctly added and follows the established pattern.
cmd/store/delete.go (2)
28-28: LGTM! Proper import addition.The flags package import is correctly added and maintains consistency with the refactoring effort.
76-79: LGTM! Correct implementation of the flags helper.The replacement properly uses
flags.SetFlagRequiredwith correct parameters and maintains the established error handling pattern.cmd/model/write.go (1)
30-30: Good addition of the flags import.The import correctly includes the new internal
flagspackage to support the centralized flag handling approach.cmd/model/list.go (2)
30-30: Good addition of the flags import.The import correctly includes the new internal
flagspackage to support the centralized flag handling approach.
116-119: Excellent refactoring to use the centralized flags utility.The implementation correctly:
- Uses the matching flag name "store-id" from the flag definition
- Provides appropriate location context for error messages
- Maintains consistent error handling with direct printing and exit behavior
cmd/store/import.go (2)
37-37: Good addition of the flags import.The import correctly includes the new internal
flagspackage to support the centralized flag handling approach.
343-346: Excellent refactoring to use the centralized flags utility.The implementation correctly:
- Uses the matching flag name "file" from the flag definition
- Provides appropriate location context for error messages
- Maintains consistent error handling with direct printing and exit behavior
cmd/model/get.go (2)
27-27: Good addition of the flags import.The import correctly includes the new internal
flagspackage to support the centralized flag handling approach.
81-84: Excellent refactoring to use the centralized flags utility.The implementation correctly:
- Uses the matching flag name "store-id" from the flag definition
- Provides appropriate location context for error messages
- Maintains consistent error handling with direct printing and exit behavior
cmd/query/list-users.go (2)
30-30: Good addition of the flags import.The import correctly includes the new internal
flagspackage to support the centralized flag handling approach.
142-145: Excellent use of the batch flag setting utility.The implementation correctly:
- Uses
SetFlagsRequiredto efficiently set multiple flags as required in a single call- All flag names match their definitions ("object", "relation", "user-filter")
- Provides appropriate location context for error messages
- Maintains consistent error handling with direct printing and exit behavior
This demonstrates a clean improvement over the previous approach of multiple individual
MarkFlagRequiredcalls.internal/flags/flags_test.go (3)
1-150: Excellent comprehensive test coverage!This test file provides thorough coverage of the
flagspackage functionality with well-structured test cases that validate:✅ Success scenarios: Both persistent and non-persistent flag operations
✅ Error handling: Missing flags with proper error message validation
✅ Edge cases: Empty/nil slices, mixed flag types
✅ Batch operations: Multiple flag processing with partial failures
✅ Error formatting: Consistent error message structure validationThe test naming conventions are clear and descriptive, following Go best practices. The use of
testifyassertions ensures robust validation, and the test structure is consistent throughout.
137-149: Well-designed test for mixed flag type scenario.This test effectively validates the behavior when trying to mark a persistent flag as required using the non-persistent method, which should fail. The assertions correctly verify that:
- Only the persistent flag operation fails
- The regular flag operation succeeds
- Error messages contain the expected context
This edge case testing demonstrates thorough understanding of the underlying Cobra flag mechanics.
123-135: Thorough validation of error joining behavior.The test effectively validates that
errors.Joinproperly combines multiple errors when all flags fail. The loop-based assertion ensures each individual error is present in the combined error message, which is crucial for debugging multiple flag issues.
rhamzeh
force-pushed
the
chore/introduce-flags-helper-for-consistency
branch
from
4ce74a1 to
22a8966
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes flag requirement enforcement by introducing a helper in internal/flags and replaces direct MarkFlagRequired/MarkPersistentFlagRequired calls across commands for consistency.
- Adds
SetFlagRequiredandSetFlagsRequiredfunctions (with tests) to handle required flags and error formatting. - Updates multiple
cmd/*files to use the new helpers instead of raw Cobra methods. - Adjusts
.golangci.yamlto allow the Cobra import for the new package.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/flags/flags.go | New helper functions for marking flags required. |
| internal/flags/flags_test.go | Tests covering various flag-required scenarios. |
| cmd/tuple/tuple.go | Replaced inline flag requirement with helper. |
| cmd/store/import.go | Switched to helper for file flag. |
| cmd/store/get.go | Switched to helper for store-id flag. |
| cmd/store/export.go | Switched to helper for store-id flag. |
| cmd/store/delete.go | Switched to helper for store-id flag. |
| cmd/query/query.go | Switched to helper for persistent store-id. |
| cmd/query/list-users.go | Consolidated three flag requirements into helper. |
| cmd/model/write.go | Switched to helper for store-id flag. |
| cmd/model/test.go | Switched to helper for tests flag. |
| cmd/model/list.go | Switched to helper for store-id flag. |
| cmd/model/get.go | Switched to helper for store-id flag. |
| .golangci.yaml | Allowed github.com/spf13/cobra in lint config. |
Comments suppressed due to low confidence (2)
internal/flags/flags_test.go:68
- [nitpick] It would be valuable to add a test that checks errors.Is(err, ErrFlagRequired) to ensure the helper properly wraps the original error for downstream error inspection.
assert.Contains(t, err.Error(), expectedPrefix)
internal/flags/flags.go:28
- fmt.Errorf supports only one %w verb. Having two %w directives will cause a compile error. Consider using %v or %s for the first error or use errors.Join to combine errors properly.
return fmt.Errorf("%w - (flag: %s, file: %s): %w", ErrFlagRequired, flag, location, err)
cmd/tuple/tuple.go
Outdated
| if err != nil { //nolint:wsl | ||
| fmt.Print(err) | ||
| if err := flags.SetFlagRequired(TupleCmd, "store-id", "cmd/tuple/tuple", true); err != nil { | ||
| fmt.Printf("%v\n", err) |
There was a problem hiding this comment.
[nitpick] Error messages should be written to stderr rather than stdout. Consider using fmt.Fprintln(os.Stderr, err) to ensure errors go to the correct output stream.
| fmt.Printf("%v\n", err) | |
| fmt.Fprintln(os.Stderr, err) |
Copilot uses AI. Check for mistakes.
Description
Introduce a helper method for marking flags as required for consistency and ease of developer use
References
Review Checklist
mainSummary by CodeRabbit