Skip to content

Modernize the entire codebase #7005

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
Merged
merged 7 commits into from
Sep 9, 2025
Merged

Modernize the entire codebase #7005

merged 7 commits into from
Sep 9, 2025

Conversation

SungJin1212
Copy link
Member

This PR modernizes the entire code base by using https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize.
Inspired by this PR: prometheus/prometheus#17092

Most of the changes are:

  • change interface{} -> any
  • use just range in the for loop
  • use min/max
  • use b.Loop() in benchmark
  • use slices.Contains

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@dosubot dosubot bot added the go Pull requests that update Go code label Sep 2, 2025
@SungJin1212 SungJin1212 changed the title Modernize enter codebaes Modernize the entire codebase Sep 2, 2025
@SungJin1212 SungJin1212 force-pushed the Modenize-entire-codebase branch from 49d999b to be4d8dc Compare September 2, 2025 04:37
yeya24
yeya24 reviewed Sep 2, 2025
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This looks promising.
Do you think we can add the tool to our CI? So we don't have to manually run it next time

danielblando
danielblando approved these changes Sep 2, 2025
View reviewed changes
Copy link
Contributor

@danielblando danielblando left a comment

Choose a reason for hiding this comment

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

Thanks

@SungJin1212 SungJin1212 force-pushed the Modenize-entire-codebase branch from be4d8dc to 1178d98 Compare September 3, 2025 01:50
@SungJin1212 SungJin1212 force-pushed the Modenize-entire-codebase branch from 35469ad to 86be435 Compare September 3, 2025 05:28
@SungJin1212
fix modernize
a1fd954 
Signed-off-by: SungJin1212 <[email protected]>
@SungJin1212
Copy link
Member Author

SungJin1212 commented Sep 3, 2025 •
edited
Loading

@yeya24
I added modernize CI to lint check.

friedrichg
friedrichg reviewed Sep 4, 2025
View reviewed changes
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

I think is all probably ok. I did review all 298 files.
I left some comments.
See if you can spot all b.ResetTImers. I bet they all can be removed too as a result of this.

Thanks for doing this!

@SungJin1212
Copy link
Member Author

@friedrichg
Thanks for the huge review!
I've removed unnecessary b.ResetTimer() and map copies in the Ingester test and deprecated the StringsContain function, which can be replaced with slices.Contains.

friedrichg
friedrichg approved these changes Sep 5, 2025
View reviewed changes
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Let's do it

pkg/ring/ring_test.go
Comment on lines -974 to -976
for id, instance := range ringDesc.Ingesters {
ringDesc.Ingesters[id] = instance
}
Copy link
Member

Choose a reason for hiding this comment

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

🙃

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 5, 2025
CharlieTLe
CharlieTLe approved these changes Sep 5, 2025
View reviewed changes
Copy link
Member

@CharlieTLe CharlieTLe left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@SungJin1212 SungJin1212 merged commit 73088a9 into cortexproject:master Sep 9, 2025
33 of 34 checks passed
@SungJin1212 SungJin1212 mentioned this pull request Sep 9, 2025
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yeya24 yeya24 yeya24 left review comments

@danielblando danielblando danielblando approved these changes

@friedrichg friedrichg friedrichg approved these changes

@CharlieTLe CharlieTLe CharlieTLe approved these changes

Assignees

No one assigned

Labels

go Pull requests that update Go code lgtm This PR has been approved by a maintainer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@SungJin1212 @danielblando @friedrichg @CharlieTLe @yeya24