-
Notifications
You must be signed in to change notification settings - Fork 0
feat(config): add support for string slice flags in traverseStruct #80
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.
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
Conversation
WalkthroughAdds handling for string slice fields in configuration traversal: registers StringSlice flags for []string and errors for slices with non-string element types. Updates tests to include a valid []string slice field and adjusts the unsupported-type test to use []int. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/config.go (1)
169-173
: Consider handling thedefault
tag for string slices.The code correctly validates string slices and registers the flag, but unlike other field types (string, int, bool, duration at lines 130-168), it doesn't parse or apply the
default
tag. Users might expect to specify comma-separated default values viadefault:"value1,value2"
.If default value support is desired, consider parsing
defaultStrValue
and splitting it:case reflect.Slice: if fieldValue.Type().Elem().Kind() != reflect.String { return fmt.Errorf("unsupported slice element type %s for field %s", fieldValue.Type().Elem().Kind(), field.Name) } - flagSet.StringSlice(prefix+tag, []string{}, description) + defaultSlice := []string{} + if defaultStrValue != "" { + defaultSlice = strings.Split(defaultStrValue, ",") + } + flagSet.StringSlice(prefix+tag, defaultSlice, description)config/config_test.go (1)
39-39
: Add assertions to verify the slice flag.The
Slice
field is added to the test struct but the test doesn't verify that the flag was registered correctly. For consistency with other flags validated in this test (lines 49-77), consider adding assertions:Add assertions after line 77:
sliceFlag := cmd.Flags().Lookup("slice") assert.NotNil(t, sliceFlag) assert.Equal(t, "This is a slice of strings", sliceFlag.Usage) assert.Equal(t, "[]", sliceFlag.DefValue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.go
(1 hunks)config/config_test.go
(2 hunks)
⏰ 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). (2)
- GitHub Check: pipe / lint / lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
config/config_test.go (1)
175-175
: LGTM!Correctly updated the unsupported type test to use
[]int
instead of[]string
, preserving the test's intent now that string slices are supported.
Summary by CodeRabbit
New Features
Improved Error Handling