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

chore: add non-empty .golangci.yml; fix whitespace lint issues #2244

Closed

Conversation

alexandear
Copy link

@alexandear alexandear commented Jan 9, 2024

This PR enables default linters for golangci-lint with whitespace linter, and fixes up whitespace lint issues.

In future, enable list may grow with new linters.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks, but no.

The whitespace cleanups, sure, let’s merge them. But past experience with maintaining an 80-entry list of linters is that we … don’t. So let’s not take on the commitment.

.golangci.yml Outdated
deadline: 5m
linters:
enable-all: true
disable:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly I don’t want to be in the business of making these kinds of decisions. The defaults have worked pretty well so far, and related projects with these kinds of long lists have often ended up with long-unmaintained lists with no clear reasons. Compare containers/storage#1579 .

This also disables one of the currently default linters… while simultaneously carrying configuration for it. Let’s not.

Copy link
Author

Choose a reason for hiding this comment

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

I changed the config to disable all linters and enable default linters and whitespace. Is it ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

7 is better than ~80, but it’s still a list that is going to get out of date.


WRT whitespace … I aesthetically like removing the empty lines, but the case is not that compelling. The result of adding that linter is that first-time external contributors use their preferred tooling, relying on standard gofmt or something like that… and the PR is later rejected, adding another roundtrip and extra friction for that contribution, for a trivial aesthetic reason.

@alexandear alexandear force-pushed the chore-add-golangci-config branch 3 times, most recently from 4ff4213 to 761b7b0 Compare January 10, 2024 11:46
Signed-off-by: Oleksandr Redko <Oleksandr_Redko@epam.com>
@@ -87,7 +87,7 @@ validate: lint
@BUILDTAGS="$(BUILDTAGS)" hack/validate.sh

lint:
$(GOBIN)/golangci-lint run --build-tags "$(BUILDTAGS)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also worry that moving the build tags to the config is going to get out of sync.

@alexandear
Copy link
Author

Won't do.

@alexandear alexandear closed this Jan 11, 2024
@alexandear alexandear deleted the chore-add-golangci-config branch January 11, 2024 19:22
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.

None yet

2 participants