Apply changes from Go's modernize tool#395
Conversation
- Use max() for cleaner index calculations in TUI logs - Use reflect.TypeFor for compile-time type checking in tests - Use strings.CutPrefix for cleaner prefix handling in trash - Use WaitGroup.Go for cleaner goroutine handling in integration tests
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughSimplifies TUI log-index calculation, swaps prefix handling to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (40.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #395 +/- ##
==========================================
- Coverage 54.43% 54.39% -0.05%
==========================================
Files 15 15
Lines 2118 2116 -2
==========================================
- Hits 1153 1151 -2
Misses 887 887
Partials 78 78
🚀 New features to boost your workflow:
|
nicholas-fedor
changed the title
modernize tool
- Remove obsolete gosec nolint directive from cmd/root.go - Clean up extra whitespace in logger integration tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/cli/tui.go (1)
764-764: Good modernization, but consider completing it across the file.Using the built-in
max()is cleaner. However, this file still uses the custommaximum()andminimum()helpers in multiple places (lines 804, 809, 816, 830, 832, 837, 908, 1053, 1055), and those functions are still defined at lines 1229-1244.Consider replacing all
maximum()/minimum()calls with the built-inmax()/min()functions and removing the now-redundant helper definitions for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/tui.go` at line 764, Replace all custom helper calls to maximum() and minimum() in this file with the Go built-in max() and min() (e.g., change maximum(x,y) -> max(x,y), minimum(x,y) -> min(x,y)), update any surrounding expressions such as the existing start := max(len(m.logs)-maxVisibleLogLines, 0) pattern to match other occurrences, and remove the now-redundant helper function definitions maximum() and minimum() from the file; ensure the replacements target the maximum/minimum identifiers and related call sites so compilation uses the built-in min/max.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/tui_test.go`:
- Around line 445-447: The type-comparison line is a tautology because
reflect.TypeFor[tea.Cmd]() compares the same type to itself; replace it with a
real runtime type comparison using the actual values: change the condition to
compare reflect.TypeOf(gotCmd) and reflect.TypeOf(tt.wantCmd) (e.g., if
reflect.TypeOf(gotCmd) != reflect.TypeOf(tt.wantCmd) { t.Errorf("model.Update()
gotCmd = %T, want %T", gotCmd, tt.wantCmd) }), referencing the variables gotCmd
and tt.wantCmd so the check in the test actually detects mismatched command
types.
- Around line 189-191: The test currently compares the same type to itself
(reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]()), which is a tautology
and never triggers; replace it with a real type check by comparing the runtime
type of the returned value to the expected type (e.g. use reflect.TypeOf(got) !=
reflect.TypeOf(tt.want)) or perform a type assertion (e.g. _, ok :=
got.(tea.Cmd); if !ok { t.Errorf("model.Init() returned %T, want tea.Cmd", got)
}) so the model.Init() result (got) is actually validated against the expected
tea.Cmd type.
In `@testing/integration/logger/logger_integration_test.go`:
- Around line 440-454: The file mixes sync.WaitGroup.Add()/Done() and the newer
wg.Go(func()) usage (see TestConcurrentLogging and the concurrent loops using
wg.Go); standardize to the wg.Go pattern across the file: replace the
Add()/defer Done() blocks in TestConcurrentLogging with wg.Go(func() { ... })
calls (removing explicit Add/Done), and ensure all goroutine spawns use wg.Go so
the file consistently relies on WaitGroup.Go.
---
Nitpick comments:
In `@internal/cli/tui.go`:
- Line 764: Replace all custom helper calls to maximum() and minimum() in this
file with the Go built-in max() and min() (e.g., change maximum(x,y) ->
max(x,y), minimum(x,y) -> min(x,y)), update any surrounding expressions such as
the existing start := max(len(m.logs)-maxVisibleLogLines, 0) pattern to match
other occurrences, and remove the now-redundant helper function definitions
maximum() and minimum() from the file; ensure the replacements target the
maximum/minimum identifiers and related call sites so compilation uses the
built-in min/max.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1752737-b545-4821-b021-c726b800dd7f
📒 Files selected for processing (4)
internal/cli/tui.gointernal/cli/tui_test.gointernal/trash/trash.gotesting/integration/logger/logger_integration_test.go
internal/cli/tui_test.go
Outdated
| if reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]() { | ||
| t.Errorf("model.Init() = %T, want %T", got, tt.want) | ||
| } |
There was a problem hiding this comment.
Test assertion is a no-op tautology.
The condition reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]() compares the same type to itself and will always be false. This effectively disables the type check assertion—the error message on line 190 will never execute.
If the intent is to verify that got has the expected type, compare reflect.TypeOf(got) against the expected type. If the test should simply confirm Init() returns a tea.Cmd (or nil), consider removing the dead code or restructuring the assertion.
Suggested fix to restore the type comparison
- if reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]() {
+ if reflect.TypeOf(got) != reflect.TypeOf(tt.want) {
t.Errorf("model.Init() = %T, want %T", got, tt.want)
}📝 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.
| if reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]() { | |
| t.Errorf("model.Init() = %T, want %T", got, tt.want) | |
| } | |
| if reflect.TypeOf(got) != reflect.TypeOf(tt.want) { | |
| t.Errorf("model.Init() = %T, want %T", got, tt.want) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/tui_test.go` around lines 189 - 191, The test currently compares
the same type to itself (reflect.TypeFor[tea.Cmd]() !=
reflect.TypeFor[tea.Cmd]()), which is a tautology and never triggers; replace it
with a real type check by comparing the runtime type of the returned value to
the expected type (e.g. use reflect.TypeOf(got) != reflect.TypeOf(tt.want)) or
perform a type assertion (e.g. _, ok := got.(tea.Cmd); if !ok {
t.Errorf("model.Init() returned %T, want tea.Cmd", got) }) so the model.Init()
result (got) is actually validated against the expected tea.Cmd type.
internal/cli/tui_test.go
Outdated
| if reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]() { | ||
| t.Errorf("model.Update() gotCmd = %T, want %T", gotCmd, tt.wantCmd) | ||
| } |
There was a problem hiding this comment.
Same tautology issue—type assertion is disabled.
Identical problem as line 189: reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]() is always false, so the type mismatch check never triggers.
Suggested fix
- if reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]() {
+ if reflect.TypeOf(gotCmd) != reflect.TypeOf(tt.wantCmd) {
t.Errorf("model.Update() gotCmd = %T, want %T", gotCmd, tt.wantCmd)
}📝 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.
| if reflect.TypeFor[tea.Cmd]() != reflect.TypeFor[tea.Cmd]() { | |
| t.Errorf("model.Update() gotCmd = %T, want %T", gotCmd, tt.wantCmd) | |
| } | |
| if reflect.TypeOf(gotCmd) != reflect.TypeOf(tt.wantCmd) { | |
| t.Errorf("model.Update() gotCmd = %T, want %T", gotCmd, tt.wantCmd) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/tui_test.go` around lines 445 - 447, The type-comparison line is
a tautology because reflect.TypeFor[tea.Cmd]() compares the same type to itself;
replace it with a real runtime type comparison using the actual values: change
the condition to compare reflect.TypeOf(gotCmd) and reflect.TypeOf(tt.wantCmd)
(e.g., if reflect.TypeOf(gotCmd) != reflect.TypeOf(tt.wantCmd) {
t.Errorf("model.Update() gotCmd = %T, want %T", gotCmd, tt.wantCmd) }),
referencing the variables gotCmd and tt.wantCmd so the check in the test
actually detects mismatched command types.
1 participant
This PR applies automated changes from Go's modernize tool.
Changes
Summary by CodeRabbit
Refactor
Tests
Style