Modernize engine#12721
Conversation
ti-chi-bot
Bot
added
release-note-none
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to adopt Go 1.22 features, including range-over-int loops, replacing interface{} with any, utilizing maps.Copy for map duplication, and optimizing string construction with strings.Builder. However, a critical compilation issue was introduced across multiple test files where the standard library sync.WaitGroup is incorrectly called with a non-existent Go method. The review comments accurately identify these compilation errors and provide correct suggestions to revert to the standard wg.Add(1) and goroutine pattern.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| wg.Go(func() { | ||
| err := jobAPISrv.listenStoppedJobs(context.Background(), stoppedJobs) | ||
| require.NoError(t, err) | ||
| }() | ||
| }) |
There was a problem hiding this comment.
The standard library sync.WaitGroup does not have a Go method. Calling wg.Go(...) will cause a compilation error. Please revert this change to use the standard wg.Add(1) and go func() { defer wg.Done(); ... }() pattern.
| wg.Go(func() { | |
| err := jobAPISrv.listenStoppedJobs(context.Background(), stoppedJobs) | |
| require.NoError(t, err) | |
| }() | |
| }) | |
| wg.Add(1) | |
| go func() { | |
| defer wg.Done() | |
| err := jobAPISrv.listenStoppedJobs(context.Background(), stoppedJobs) | |
| require.NoError(t, err) | |
| }() |
There was a problem hiding this comment.
How to make Gemini shut up about these (*WaitGroup).Go changes?
There was a problem hiding this comment.
As Gemini Code Assist is going the way of the Dodo this isn't worth the effort.
kennytm
left a comment
kennytm
left a comment
There was a problem hiding this comment.
iirc engine is no longer maintained though 🤔
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kennytm The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
2 participants
What problem does this PR solve?
Issue Number: ref #12691
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note