Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Run gopls modernize on the codebase #33739

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Run gopls modernize on the codebase #33739

wants to merge 4 commits into from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Feb 27, 2025 •
edited
Loading

gopls has a internal modernize code action which we can run on the CLI which I have done here:

go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix ./...

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 27, 2025
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/migrations modifies/internal labels Feb 27, 2025
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 27, 2025
@silverwind silverwind added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 27, 2025
@silverwind
Copy link
Member Author

silverwind commented Feb 27, 2025 •
edited
Loading

It seems that at least one fix that was applied was unsafe. I suppose this is a bug in the module:

Error: services/actions/rerun.go:25:36: in call to slices.ContainsFunc, type func(values ...string) bool of rerunJobsIDSet.Contains does not match func(E) bool

Tool output:

$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix ./...

-: # code.gitea.io/gitea/services/actions
services/actions/rerun.go:25:36: in call to slices.ContainsFunc, type func(values ...string) bool of rerunJobsIDSet.Contains does not match func(E) bool
/Users/silverwind/git/gitea/services/actions/rerun.go:25:36: in call to slices.ContainsFunc, type func(values ...string) bool of rerunJobsIDSet.Contains does not match func(E) bool
-: # code.gitea.io/gitea/services/actions [code.gitea.io/gitea/models/db.test]
services/actions/rerun.go:25:36: in call to slices.ContainsFunc, type func(values ...string) bool of rerunJobsIDSet.Contains does not match func(E) bool
-: # code.gitea.io/gitea/services/actions [code.gitea.io/gitea/services/actions.test]
services/actions/rerun.go:25:36: in call to slices.ContainsFunc, type func(values ...string) bool of rerunJobsIDSet.Contains does not match func(E) bool
/Users/silverwind/git/gitea/services/actions/rerun.go:25:36: in call to slices.ContainsFunc, type func(values ...string) bool of rerunJobsIDSet.Contains does not match func(E) bool
exit status 1

@silverwind silverwind marked this pull request as draft February 27, 2025 11:11
@silverwind
Copy link
Member Author

silverwind commented Feb 27, 2025 •
edited
Loading

Given the "internal" nature of this thing, maybe we should just take this as a one-time refactoring action for now. Maybe in the future modernize will become part of the official gopls CLI.

delvh
delvh reviewed Feb 27, 2025
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your assessment, it is fine as a one time action, but we should not add it to the CI while it is inofficial.

contrib/environment-to-ini/environment-to-ini.go
Comment on lines 82 to +83
// the config system may change the environment variables, so get a copy first, to be used later
env := append([]string{}, os.Environ()...)
env := os.Environ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was also erroneous.
Although the code itself should probably use the maps.Clone function anyway…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reported upstream: golang/go#72009

Copy link
Member Author

@silverwind silverwind Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no, I think the fix is correct. os.Environ() returns string[], so maps.Clone will not work.

I think it does not make sense to clone a string[] here, or do you? I think even if the environment changes after this call, the value will stay the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a gopls bug. os.Environ always returns a new slice, so no need to "clone"

modules/setting/mirror.go
} else {
Mirror.DefaultInterval = time.Hour * 8
}
Mirror.DefaultInterval = max(time.Hour*8, Mirror.MinInterval)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the tool, that's what the code is doing.
However, it's not what it should be doing:
I think the code is supposed to be

Suggested change
Mirror.DefaultInterval = max(time.Hour*8, Mirror.MinInterval)
Mirror.DefaultInterval = Mirror.MinInterval

Comparing against the default value looks strange

Copy link
Member Author

@silverwind silverwind Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope to keep behaviour changes to a minimum in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, avoid unnecessary changes.

@silverwind
Copy link
Member Author

Okay, I have reverted all the tooling changes and added the command to run to the PR message above for future reference.

@silverwind silverwind changed the title Add gopls modernize formatting Run gopls modernize on the codebase Feb 27, 2025
@silverwind silverwind mentioned this pull request Feb 27, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@delvh delvh delvh left review comments

Assignees
No one assigned
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code modifies/internal modifies/migrations size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@silverwind @wxiaoguang @delvh @GiteaBot