Skip to content

Run gopls modernize on codebase #34751

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

Merged
merged 3 commits into from
Jun 18, 2025
Merged

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jun 17, 2025

Continue #33739. I ran modernize 3 times until it had nothing to fix, followed by fmt and fixed one lint issue. 🙏 that CI passes.

Recent modernize fixes: https://github.com/golang/tools/commits/master/gopls/internal/analysis/modernize

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 17, 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 Jun 17, 2025
@silverwind
Copy link
Member Author

Test failure TestChangeRepoFilesForUpdateWithFileRename is unrelated.

break
}
if slices.ContainsFunc(actions, glob.MustCompile(val, '/').Match) {
matched = true
Copy link
Member

Choose a reason for hiding this comment

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

It can be break directly here.

break
}
if slices.ContainsFunc(actions, glob.MustCompile(val, '/').Match) {
matched = true
Copy link
Member

Choose a reason for hiding this comment

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

The same as above

@silverwind
Copy link
Member Author

BTW feel free to push any changes directly to this branch.

isCurDBTypeSupported = true
break
}
if slices.Contains(setting.SupportedDatabaseTypes, curDBType) {
Copy link
Member

Choose a reason for hiding this comment

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

The 4 lines could be merged.

find = true
break
}
if slices.Contains(loadRepoIDs, result.RepoID) {
Copy link
Member

Choose a reason for hiding this comment

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

merge the 4 lines

}
expectedIssueCount := min(
// from the fixtures
20, setting.UI.IssuePagingNum)
Copy link
Member Author

Choose a reason for hiding this comment

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

This formatting is not ideal, I will fix.

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 @lunny, otherwise LGTM

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 17, 2025
@wxiaoguang
Copy link
Contributor

Please wait for Refactor some file edit related code #34744 , otherwise there will be huge conflicts which are difficult to resolve.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

It's large enough, and the auto-fixed code is good enough.

Let's leave the manual fixes to the future, to avoid introducing regressions.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 18, 2025
@wxiaoguang wxiaoguang enabled auto-merge (squash) June 18, 2025 01:33
}
expectedIssueCount := min(
// from the fixtures
20, setting.UI.IssuePagingNum)
Copy link
Contributor

Choose a reason for hiding this comment

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

Mark another manual fix (let's do in the following up PRs)

}
expectedIssueCount := min(
// from the fixtures
20, setting.UI.IssuePagingNum)
Copy link
Contributor

Choose a reason for hiding this comment

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

mark

}
expectedIssueCount := min(
// from the fixtures
20, setting.UI.IssuePagingNum)
Copy link
Contributor

Choose a reason for hiding this comment

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

mark

break
}
if slices.Contains(loadRepoIDs, result.RepoID) {
find = true
Copy link
Contributor

Choose a reason for hiding this comment

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

mark

@@ -149,8 +149,8 @@ func loadMarkupFrom(rootCfg ConfigProvider) {
func newMarkupSanitizer(name string, sec ConfigSection) {
rule, ok := createMarkupSanitizerRule(name, sec)
if ok {
if strings.HasPrefix(name, "sanitizer.") {
names := strings.SplitN(strings.TrimPrefix(name, "sanitizer."), ".", 2)
if after, ok0 := strings.CutPrefix(name, "sanitizer."); ok0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

mark

@wxiaoguang wxiaoguang merged commit 1f35435 into go-gitea:main Jun 18, 2025
26 checks passed
@GiteaBot GiteaBot added this to the 1.25.0 milestone Jun 18, 2025
@a1012112796
Copy link
Member

maybe we can use this tool in ci steps?

@lunny
Copy link
Member

lunny commented Jun 18, 2025

maybe we can use this tool in ci steps?

gopls is slow enough.

@wxiaoguang wxiaoguang deleted the modernize2 branch June 18, 2025 02:35
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 18, 2025
* giteaofficial/main:
  Remove unused param `doer` (go-gitea#34545)
  Improve alignment of commit status icon on commit page (go-gitea#34750)
  Run `gopls modernize` on codebase (go-gitea#34751)
  Refactor some file edit related code (go-gitea#34744)
  [skip ci] Updated translations via Crowdin
  Fix ghost user in feeds when pushing in an actions, it should be gitea-actions (go-gitea#34703)
  upgrade orgmode to v1.8.0 (go-gitea#34721)
  Support title and body query parameters for new PRs (go-gitea#34537)
  Improve nuget/rubygems package registries (go-gitea#34741)
  Replace update repository function in some places (go-gitea#34566)
  remove unnecessary duplicate code (go-gitea#34733)
  fix: prevent double markdown link brackets when pasting URL (go-gitea#34745)
  Fix JS error for "select" dropdown (go-gitea#34743)
  [skip ci] Updated translations via Crowdin
  Allow renaming/moving binary/LFS files in the UI (go-gitea#34350)
  Clean bindata (go-gitea#34728)
  Refactor container and UI (go-gitea#34736)
  Prevent duplicate form submissions when creating forks (go-gitea#34714)
@silverwind
Copy link
Member Author

silverwind commented Jun 18, 2025

maybe we can use this tool in ci steps?

Yes that is the plan, at least as a initial lint-only step. Once we upgrade gopls, the gopls check command will enforce these fixes. Maybe later, they could be rolled into make fmt.

gopls is slow enough.

Yes, gopls check is slow, but I guess these additional checks won't slow it down much more. Ideally such checks would execute as part of golangci-lint, but first those two projects need to sort out their issues, see golangci/golangci-lint#686 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants