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

Refactor golangci-lint config and remove redundant nolints #1486

Merged
merged 2 commits into from
Feb 11, 2023
Merged

Refactor golangci-lint config and remove redundant nolints #1486

merged 2 commits into from
Feb 11, 2023

Conversation

alexandear
Copy link
Contributor

@alexandear alexandear commented Feb 9, 2023

This PR does the following:

  • add additional linter run options in GitHub lint's workflow;
  • remove unused //nolint comments;
  • use golangci-lint-action because it's shorted and caches data.

Options:

  • --verbose for clear visibility of which set of linters is run;
  • --enable=gochecknoinits enables gochecknoinits (the repo has one init() which already nolinted);
  • --enable=nolintlint enables linter that reports redundant //nolint comments.

structcheck linter is deprecated and automatically disabled by golangci-lint.

@alexandear alexandear changed the title Refactor golangci-lint config Refactor golangci-lint config and remove redundant nolints Feb 9, 2023
- Use golangci-lint-action for GitHub workflow.
- Add additional golangci-lint run options.
- Remove unused nolint directives.
@erikdubbelboer
Copy link
Collaborator

I'm sorry but I don't agree with removing nolint:errcheck. I know that happens a lot in tests. But I would still prefer to mark each line individually as it makes the reader aware that an error is ignored here. Without it it's hard to see if this happens or not. I also wouldn't want any new code to be added later where we forget to check there error and the linter doesn't complain.

@alexandear
Copy link
Contributor Author

alexandear commented Feb 10, 2023

I'm sorry but I don't agree with removing nolint:errcheck. I know that happens a lot in tests. But I would still prefer to mark each line individually as it makes the reader aware that an error is ignored here. Without it it's hard to see if this happens or not. I also wouldn't want any new code to be added later where we forget to check there error and the linter doesn't complain.

@erikdubbelboer Thanks for the comment. Partially agree. Could you comment on the changed code lines? Because I don't understand what should I do. Reset only //nolint:errcheck changes?

@alexandear
Copy link
Contributor Author

Maybe we should rewrite tc.Close() \\nolint:errcheckto _ = tc.Close()? This states explicitly we ignore the returned error.

@erikdubbelboer
Copy link
Collaborator

Maybe we should rewrite tc.Close() \\nolint:errcheckto _ = tc.Close()? This states explicitly we ignore the returned error.

Yes, please remove --exclude-use-default as it disables a lot more checks than just errcheck. And then just add _ = to lines that fail on errcheck.

@alexandear
Copy link
Contributor Author

@erikdubbelboer done

@erikdubbelboer erikdubbelboer merged commit 934f04e into valyala:master Feb 11, 2023
@erikdubbelboer
Copy link
Collaborator

Thanks!

@alexandear alexandear deleted the refactor-linter-config branch February 11, 2023 07:40
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.

2 participants