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.

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

bump up golangci-lint to v1.52.2 #3796

Merged
merged 1 commit into from
Apr 28, 2023
Merged

Conversation

ktock
Copy link
Collaborator

@ktock ktock commented Apr 13, 2023

This PR bumps up golangci-lint to v1.52.2 https://github.com/golangci/golangci-lint/releases/tag/v1.52.2
With this version, the linter fails with a lot of unused-parameter failures. Though this PR fixes the code to pass the linter, maybe we can disable it if unnecessary.

@ktock ktock force-pushed the golangci-lint-v1.52 branch from 8ba005e to 0386ef9 Compare April 14, 2023 01:43
@ktock ktock marked this pull request as ready for review April 17, 2023 01:34
@jedevc
Copy link
Member

jedevc commented Apr 17, 2023

I don't have strong opinions on enabling unused-parameter - so happy to take the refactoring.

Could you reorder the commits so that the refactor comes before the bump, so that the linter can pass on each commit?

@ktock ktock force-pushed the golangci-lint-v1.52 branch from 0386ef9 to d50906c Compare April 17, 2023 10:26
cache/blobs.go Outdated
}

return nil
return sr.commitMetadata()
Copy link
Member

Choose a reason for hiding this comment

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

too strict

@@ -11,6 +11,6 @@ import (
ocispecs "github.com/opencontainers/image-spec/specs-go/v1"
)

func needsForceCompression(ctx context.Context, cs content.Store, source ocispecs.Descriptor, refCfg config.RefConfig) bool {
func needsForceCompression(_ context.Context, _ content.Store, _ ocispecs.Descriptor, refCfg config.RefConfig) bool {
Copy link
Member

Choose a reason for hiding this comment

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

too strict

@ktock ktock marked this pull request as draft April 19, 2023 08:18
@ktock ktock force-pushed the golangci-lint-v1.52 branch from d50906c to b96586f Compare April 20, 2023 03:10
@ktock
Copy link
Collaborator Author

ktock commented Apr 20, 2023

Looks like mgechev/revive#799 added the following rules which make revive very strict.

  • if-return
  • empty-block
  • superfluous-else
  • unused-parameter
  • unreachable-code
  • redefines-builtin-id

Let's disable all of them until the become needing them.

@ktock ktock force-pushed the golangci-lint-v1.52 branch from b96586f to 7c66dfb Compare April 20, 2023 03:20
@ktock ktock marked this pull request as ready for review April 20, 2023 03:56
@jedevc
Copy link
Member

jedevc commented Apr 26, 2023

@ktock I think we can exclude the tests in revive using the issues block, e.g. ddc60ed.

I'd rather keep using an exclude pattern instead of explicitly listing everything, does that SGTY?

@ktock
Copy link
Collaborator Author

ktock commented Apr 26, 2023

Thanks, SGTM.

@jedevc jedevc reopened this Apr 27, 2023
@jedevc
Copy link
Member

jedevc commented Apr 27, 2023

@ktock woops sorry, didn't mean to close this, looks like it got accidentally linked to another PR 😢

Signed-off-by: Kohei Tokunaga <[email protected]>
@ktock ktock force-pushed the golangci-lint-v1.52 branch from 7c66dfb to 60776f2 Compare April 27, 2023 14:54
@ktock
Copy link
Collaborator Author

ktock commented Apr 27, 2023

Sorry, I thought that PR fixed this 🤦‍♂️

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

unused-parameter
unreachable-code

look meaningful and we should try to enable them in follow-up

@ktock ktock requested a review from jedevc April 28, 2023 11:10
@AkihiroSuda AkihiroSuda merged commit c563db6 into moby:master Apr 28, 2023
@ktock ktock deleted the golangci-lint-v1.52 branch April 28, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants