♻️ refactor: remove redundant modernize workflow#203
Conversation
b172c5e to
8c35e61
Compare
📝 WalkthroughWalkthroughThis PR modernizes Go code patterns by removing deprecated loop-variable capture workarounds, upgrading to newer standard library APIs (atomic types, collection utilities, reflection constants), and cleaning up linter suppressions across the codebase. ChangesGo Modernization and Loop Pattern Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ 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 |
There was a problem hiding this comment.
Code Review
This pull request modernizes the codebase by adopting Go 1.22 features, such as the new 'for range' integer syntax and the removal of redundant loop variable captures. It also updates the code to use modern APIs, including 'atomic.Uint32', 'reflect.Pointer', and the 'slices' package for sorting, while adding the 'modernize' linter to the configuration. Feedback was provided regarding the global disabling of the 'package-naming' linter, which may reduce maintainability and appears to be outside the scope of the modernization effort.
There was a problem hiding this comment.
Pull request overview
This PR integrates the modernize linter into the repository’s golangci-lint configuration and removes the dedicated Modernize workflow by fixing the newly surfaced modernize-related issues across the codebase.
Changes:
- Enable the
modernizelinter in.golangci.ymland pingolangci-linttov2.12.2in the lint workflow. - Remove the standalone
.github/workflows/modernize.ymlworkflow as redundant. - Apply modernize-driven refactors across utils (atomic typed values,
slices.Sort, updated loop patterns, and test loop variable capture cleanups).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
time.go |
Switch cached timestamp to atomic.Uint32 and use typed atomic methods. |
strings/case_test.go |
Simplify ASCII iteration using modern range-over-integer syntax. |
format.go |
Remove now-redundant gosec suppressions on bounded indexing/conversions. |
file.go |
Replace sort.Strings with slices.Sort and update imports. |
convert.go |
Modernize reflection pointer-kind check. |
convert_test.go |
Remove no-longer-needed range-variable capture with parallel subtests. |
common.go |
Add modernize suppression for a reverse-loop false positive in IP increment logic. |
common_test.go |
Use range-over-integer loops and remove redundant range-variable captures. |
byteseq_test.go |
Remove redundant range-variable captures in parallel tests/benchmarks. |
.golangci.yml |
Enable modernize and disable revive’s package-naming rule. |
.github/workflows/modernize.yml |
Remove redundant Modernize workflow. |
.github/workflows/lint.yml |
Pin golangci-lint to v2.12.2 in the reusable lint workflow call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #203 +/- ##
=======================================
Coverage 84.08% 84.08%
=======================================
Files 14 14
Lines 1150 1150
=======================================
Hits 967 967
Misses 152 152
Partials 31 31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@convert.go`:
- Around line 216-218: The code calls reflect.ValueOf(arg).Kind() without
checking that the reflect.Value is valid, which causes ToString(nil) to panic;
update the ToString function to guard before calling Kind by checking
reflect.ValueOf(arg).IsValid() (or test arg == nil) and handling nil/invalid
values (return empty string or the intended zero value) before proceeding to
inspect pointers with Kind() and rv.IsNil(); ensure the existing branch that
handles reflect.Pointer && !rv.IsNil() still runs only after the validity check.
🪄 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: 33f69a7c-058a-4990-98ef-8f936d62e794
⛔ Files ignored due to path filters (3)
.github/workflows/lint.ymlis excluded by!**/*.yml.github/workflows/modernize.ymlis excluded by!**/*.yml.golangci.ymlis excluded by!**/*.yml
📒 Files selected for processing (9)
byteseq_test.gocommon.gocommon_test.goconvert.goconvert_test.gofile.goformat.gostrings/case_test.gotime.go
💤 Files with no reviewable changes (2)
- convert_test.go
- byteseq_test.go
📜 Review details
🔇 Additional comments (7)
common.go (1)
121-121: LGTM!format.go (1)
105-105: LGTM!Also applies to: 109-109, 148-148, 152-152, 172-172, 176-176, 178-178, 213-213
time.go (1)
10-10: LGTM!Also applies to: 19-19, 26-26, 38-38
file.go (1)
10-10: LGTM!Also applies to: 49-49
common_test.go (2)
63-71: LGTM!
106-114: LGTM!strings/case_test.go (1)
65-73: LGTM!
|
|
@gaby thank you, updated. |
This PR enables the
modernizelinter in the golangci-lint configuration. It also removes the now-redundant "Modernize Lint" workflow and fixesmodernizelint issues.These changes require upgrading golangci-lint to v2.12.2 and Go to 1.25.
Summary by CodeRabbit
Release Notes
Tests
Refactor
Chores