Conversation
WalkthroughThe pull request refactors formatter control flow and adds type-based formatting options. It consolidates JSON and YAML formatter paths to use a single FormatValue method, introduces TypeOptions to control slice-to-table/tree conversion behavior, adds reflection utilities, and updates logging behavior. Several tests are disabled. Changes
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
formatters/tests/sorting_test.go (1)
11-45: Re-enableTestSortRowsor document the reason for disabling it.The test is disabled via the
XTestSortRowsnaming convention.SortRowsis used in production code (formatters/parser.go, lines 507 and 934) and has no other direct test coverage. Re-enable the test to maintain coverage, or uset.Skip("reason")and explain why the test was disabled.
🤖 Fix all issues with AI agents
In `@formatters/parser.go`:
- Around line 618-625: The slice/array branch can fall through to
ParseStructSchema(val) when both opt.SkipTree and opt.SkipTable are true; update
the slice handling in the block that checks val.Kind() (and uses
hasTreeStructure, convertSliceToTreeData, convertSliceToPrettyData) to return
early when both skip flags are set so you do not call ParseStructSchema on a
slice—e.g., detect if opt.SkipTree && opt.SkipTable and return a sensible empty
value or error (such as nil or a specific sentinel) from the current function to
prevent the fall-through to ParseStructSchema.
In `@formatters/reflect.go`:
- Around line 80-107: The ToSlice function treats a single empty input slice as
a failed conversion because it returns (nil, false) when len(result) == 0;
modify the branch handling a single slice/array (inside ToSlice) so that after
iterating elements it returns an empty []T and true for valid conversions even
when there are zero elements (i.e., return result (initialized as []T) and true
instead of using len(result) > 0), while preserving the existing failure returns
for non-convertible element cases in the same reflect.Value slice/array handling
code path.
- Around line 64-70: The current code assumes all flattened elements share the
same concrete type by taking elemType := flattened[0].Type() and then using
reflect.MakeSlice/Set, which will panic on mixed types; update the logic to
first scan flattened and verify every elem.Type() equals elemType, and if any
mismatch is found, fall back to creating a slice of interface{} (use
reflect.SliceOf(reflect.TypeOf((*interface{})(nil)).Elem())) and set elements
there, or alternatively return an explicit error; ensure this check references
flattened, elemType, newSlice, reflect.MakeSlice, reflect.SliceOf and Set so the
homogeneous-case remains unchanged and mixed-type cases are handled safely.
- Around line 207-214: The code creates an unnecessary lowerName variable and
then passes it to strings.EqualFold, which already does case-insensitive
comparison; in the field lookup loop (using typ.NumField, typ.Field, val.Field)
remove the precomputed lowerName and change the comparison to
strings.EqualFold(field.Name, name) (or, if you prefer explicit lowercasing, use
strings.ToLower(field.Name) == strings.ToLower(name)); update the loop to only
use one approach and remove the unused lowerName variable for clarity.
🧹 Nitpick comments (4)
api/text.go (1)
217-241: Non-deterministic output order and inconsistent empty-string filtering.Two issues:
Non-deterministic ordering: Iterating over
styleSetmap (line 232) produces unpredictable output order, making tests flaky and debugging harder.Inconsistent empty-string handling: The first loop (lines 220-222) doesn't filter empty strings, but the second loop does (line 226). Input like
"a b"(double space) will add an empty string to the set.♻️ Suggested fix with deterministic ordering and consistent filtering
func uniqueStyles(existing string, styles ...string) string { styleSet := make(map[string]struct{}) if existing != "" { for _, s := range strings.Split(existing, " ") { - styleSet[s] = struct{}{} + if s != "" { + styleSet[s] = struct{}{} + } } } for _, style := range styles { for _, s := range strings.Split(style, " ") { if s != "" { styleSet[s] = struct{}{} } } } - uniq := "" - for s := range styleSet { - if uniq == "" { - uniq = s - } else { - uniq += " " + s - } - } - - return uniq + result := make([]string, 0, len(styleSet)) + for s := range styleSet { + result = append(result, s) + } + sort.Strings(result) + return strings.Join(result, " ") }formatters/tests/formatters_test.go (1)
46-54: Minor: Extraneous blank line inside the if block.Line 50 introduces a blank line between the error message and the closing brace. This appears unintentional and slightly inconsistent with the style in other validation blocks.
Suggested fix
Validate: func(t *testing.T, output string) { // Check that it contains formatted fields if !strings.Contains(output, "id: TEST-001") { t.Errorf("Pretty formatter should display ID field") - }formatters/tests/map_fields_test.go (1)
54-85: Consider tracking the disabled test with an issue.The
ParseDataWithSchemasubtest is commented out with a FIXME citing "API changes." Commented-out tests tend to become stale over time. Consider either:
- Creating an issue to track re-enabling this test once API changes stabilize
- Removing the test entirely if it's no longer relevant
- Using
t.Skip("reason")instead of commenting, which preserves the test in the suite and documents the skip reason in test outputWould you like me to open an issue to track re-enabling this test?
formatters/tests/tree_test.go (1)
128-153: Test disabled via X prefix—consider usingt.Skip()instead.Renaming
TestCustomRenderFunctiontoXTestCustomRenderFunctioneffectively disables the test, but this approach has drawbacks:
- No indication of why the test was disabled
- The test won't appear in test output at all, making it easy to forget
- Harder to track disabled tests across the codebase
Using
t.Skip("reason for skipping")would preserve visibility in test runs and document the rationale.Alternative approach
-func XTestCustomRenderFunction(t *testing.T) { +func TestCustomRenderFunction(t *testing.T) { + t.Skip("Disabled pending API changes - see issue `#XX`") // Register a test render function
| if val.Kind() == reflect.Slice || val.Kind() == reflect.Array { | ||
| if hasTreeStructure(val) { | ||
| if !opt.SkipTree && hasTreeStructure(val) { | ||
| return convertSliceToTreeData(val) | ||
| } | ||
| return convertSliceToPrettyData(val) | ||
| if !opt.SkipTable { | ||
| return convertSliceToPrettyData(val) | ||
| } | ||
| } |
There was a problem hiding this comment.
Slice falls through to struct parsing when both skip flags are true.
When SkipTree=true and SkipTable=true for a slice/array, neither branch returns, causing the code to fall through to ParseStructSchema(val) on line 628, which will fail for slices.
Consider adding a fallback or returning early for slices when both are skipped:
Proposed fix
if val.Kind() == reflect.Slice || val.Kind() == reflect.Array {
if !opt.SkipTree && hasTreeStructure(val) {
return convertSliceToTreeData(val)
}
if !opt.SkipTable {
return convertSliceToPrettyData(val)
}
+ // Both table and tree are skipped - return raw data
+ return &api.PrettyData{
+ Schema: &api.PrettyObject{Fields: []api.PrettyField{}},
+ Original: data,
+ }, nil
}🤖 Prompt for AI Agents
In `@formatters/parser.go` around lines 618 - 625, The slice/array branch can fall
through to ParseStructSchema(val) when both opt.SkipTree and opt.SkipTable are
true; update the slice handling in the block that checks val.Kind() (and uses
hasTreeStructure, convertSliceToTreeData, convertSliceToPrettyData) to return
early when both skip flags are set so you do not call ParseStructSchema on a
slice—e.g., detect if opt.SkipTree && opt.SkipTable and return a sensible empty
value or error (such as nil or a specific sentinel) from the current function to
prevent the fall-through to ParseStructSchema.
| // Create a new slice with the flattened elements | ||
| // Determine the element type from the first flattened element | ||
| elemType := flattened[0].Type() | ||
| newSlice := reflect.MakeSlice(reflect.SliceOf(elemType), len(flattened), len(flattened)) | ||
| for i, elem := range flattened { | ||
| newSlice.Index(i).Set(elem) | ||
| } |
There was a problem hiding this comment.
Type mismatch in flattened elements could cause panic.
Line 67 determines the element type from flattened[0], and line 69 uses Set() to assign each element. If the inner slices contain elements of different concrete types (e.g., mixing int and string in [][]interface{}), Set() will panic due to type mismatch.
This may be acceptable if the function's contract requires homogeneous types, but consider documenting this precondition or adding defensive type checking.
🤖 Prompt for AI Agents
In `@formatters/reflect.go` around lines 64 - 70, The current code assumes all
flattened elements share the same concrete type by taking elemType :=
flattened[0].Type() and then using reflect.MakeSlice/Set, which will panic on
mixed types; update the logic to first scan flattened and verify every
elem.Type() equals elemType, and if any mismatch is found, fall back to creating
a slice of interface{} (use
reflect.SliceOf(reflect.TypeOf((*interface{})(nil)).Elem())) and set elements
there, or alternatively return an explicit error; ensure this check references
flattened, elemType, newSlice, reflect.MakeSlice, reflect.SliceOf and Set so the
homogeneous-case remains unchanged and mixed-type cases are handled safely.
| func ToSlice[T any](data ...any) ([]T, bool) { | ||
| if len(data) == 0 { | ||
| return nil, false | ||
| } | ||
|
|
||
| var result []T | ||
|
|
||
| // Case 1: Single argument that is already a slice | ||
| if len(data) == 1 { | ||
| val := reflect.ValueOf(data[0]) | ||
| if val.Kind() == reflect.Slice || val.Kind() == reflect.Array { | ||
| // It's a slice, try to convert each element | ||
| for i := 0; i < val.Len(); i++ { | ||
| elem := val.Index(i) | ||
| if elem.CanInterface() { | ||
| if typed, ok := elem.Interface().(T); ok { | ||
| result = append(result, typed) | ||
|
|
||
| } else { | ||
|
|
||
| return nil, false // Not all elements are T | ||
| } | ||
| } else { | ||
|
|
||
| return nil, false | ||
| } | ||
| } | ||
| return result, len(result) > 0 |
There was a problem hiding this comment.
Edge case: Empty input slice returns (nil, false).
When a single empty slice is passed (e.g., ToSlice[int]([]int{})), the function returns (nil, false) due to line 107's condition len(result) > 0. This might be unexpected—an empty slice is a valid conversion result. Consider whether ([]T{}, true) would be more appropriate for empty inputs.
🤖 Prompt for AI Agents
In `@formatters/reflect.go` around lines 80 - 107, The ToSlice function treats a
single empty input slice as a failed conversion because it returns (nil, false)
when len(result) == 0; modify the branch handling a single slice/array (inside
ToSlice) so that after iterating elements it returns an empty []T and true for
valid conversions even when there are zero elements (i.e., return result
(initialized as []T) and true instead of using len(result) > 0), while
preserving the existing failure returns for non-convertible element cases in the
same reflect.Value slice/array handling code path.
| // Try case-insensitive match | ||
| lowerName := strings.ToLower(name) | ||
| for i := 0; i < typ.NumField(); i++ { | ||
| field := typ.Field(i) | ||
| if strings.EqualFold(field.Name, lowerName) { | ||
| return val.Field(i) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Case-insensitive comparison uses pre-lowered name incorrectly.
Line 208 converts name to lowercase, but line 211 uses strings.EqualFold which is already case-insensitive. The comparison strings.EqualFold(field.Name, lowerName) will work, but the lowerName variable is unnecessary since EqualFold handles case differences.
More importantly, if the intent was to do a simple lowercase comparison without EqualFold, using strings.ToLower(field.Name) == lowerName would be correct. The current code works but is redundant.
Suggested simplification
// Try case-insensitive match
- lowerName := strings.ToLower(name)
for i := 0; i < typ.NumField(); i++ {
field := typ.Field(i)
- if strings.EqualFold(field.Name, lowerName) {
+ if strings.EqualFold(field.Name, name) {
return val.Field(i)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Try case-insensitive match | |
| lowerName := strings.ToLower(name) | |
| for i := 0; i < typ.NumField(); i++ { | |
| field := typ.Field(i) | |
| if strings.EqualFold(field.Name, lowerName) { | |
| return val.Field(i) | |
| } | |
| } | |
| // Try case-insensitive match | |
| for i := 0; i < typ.NumField(); i++ { | |
| field := typ.Field(i) | |
| if strings.EqualFold(field.Name, name) { | |
| return val.Field(i) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@formatters/reflect.go` around lines 207 - 214, The code creates an
unnecessary lowerName variable and then passes it to strings.EqualFold, which
already does case-insensitive comparison; in the field lookup loop (using
typ.NumField, typ.Field, val.Field) remove the precomputed lowerName and change
the comparison to strings.EqualFold(field.Name, name) (or, if you prefer
explicit lowercasing, use strings.ToLower(field.Name) == strings.ToLower(name));
update the loop to only use one approach and remove the unused lowerName
variable for clarity.
|
🎉 This PR is included in version 1.12.2 |
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.