Skip to content

Conversation

@cce
Copy link
Contributor

@cce cce commented Oct 27, 2025

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 fix command 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.

@cce cce force-pushed the update-modernize branch from bf8e842 to c6be1b3 Compare October 27, 2025 15:36
@codecov
Copy link

codecov bot commented Oct 27, 2025

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
106384 2 106382 2481
View the top 2 failed test(s) by shortest run time
::TestMain
Stack Traces | 0s run time
FAIL	github..../test/e2e-go/restAPI [build failed]
::TestMain
Stack Traces | 0s run time
FAIL	github..../go-algorand/ledger/simulation [build failed]

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@gmalouf
Copy link
Contributor

gmalouf commented Oct 27, 2025

@cce some linter errors to work through

@cce cce requested a review from Copilot October 27, 2025 17:25
Copy link
Contributor

Copilot AI left a 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.Split with strings.SplitSeq, reflect.TypeOf with reflect.TypeFor, and context.WithCancel with t.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.

@cce cce force-pushed the update-modernize branch from 578deca to 9239652 Compare October 27, 2025 17:28
Copy link
Contributor

@gmalouf gmalouf left a 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/.

@gmalouf gmalouf merged commit 495da8c into algorand:master Oct 29, 2025
69 of 73 checks passed
@cce cce deleted the update-modernize branch October 29, 2025 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants