-
Notifications
You must be signed in to change notification settings - Fork 518
CI: update modernize and auto-fix a few issues #6476
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
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
@cce some linter errors to work through |
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.
Pull Request Overview
This PR updates the CI tooling to use the new location for the Go modernize tool and applies auto-fixes for code modernization. The changes include updating the Makefile to use the new modernize tool path and configuring which linters to run, while also removing large portions of unused code from the algofix tool that were copied from an older version of the go fix command.
Key changes:
- Updated Makefile to use new modernize tool location with explicit linter configuration
- Applied automated modernizations: replaced
strings.Splitwithstrings.SplitSeq,reflect.TypeOfwithreflect.TypeFor, andcontext.WithCancelwitht.Context() - Removed ~800 lines of unused code from algofix including typecheck.go, import tests, and unused helper functions
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Updated modernize tool path and linter configuration |
| Multiple test files | Applied strings.SplitSeq and t.Context() modernizations |
| Multiple source files | Applied reflect.TypeFor modernization |
| cmd/algofix/*.go | Removed unused typecheck, import test code, and helper functions |
| cmd/algofix/deadlock_test.go | Replaced string(buf.Bytes()) with buf.String() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
This makes sense to me, enjoyed reading the story behind TypeFor on https://blog.carlana.net/post/2024/golang-reflect-type-for/.
Summary
The go maintainers have deprecated the old paths for running modernize, and moved to a new location. In https://go.dev/issue/71859 they detail how this will end up in an official
go fixcommand in 1.26.This PR updates the Makefile to run the new modernize tool and flips our list of explicitly-enabled linters to an explicitly-disabled list, so new ones will show up as they are released.
Touching cmd/algofix/fix.go set off the "unused" linter, and fixing that revealed that huge chunks of algofix are unused (copy-pasted from an old version of the go fix tool), so this removes all that unused code.
Test Plan
Existing tests should pass.
Closes #6465.