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 golangci-lint as linter #317

Merged
merged 11 commits into from
Oct 21, 2021
Merged

Conversation

mavogel
Copy link
Contributor

@mavogel mavogel commented May 26, 2021

Description

Adds golangci-lint as linter, with Makefile target and GH action.
We could fixe all the issues and/or disable certain linters on the way: @maxekman WDYT?

Affected Components

  • all

Related Issues

#199

Steps to test and verify

  1. make setup
  2. make lint

@maxekman
Copy link
Member

Looks interesting! Will take a look soon.

@coveralls
Copy link

coveralls commented Jun 7, 2021

Coverage Status

Coverage increased (+0.1%) to 69.231% when pulling 1fa17e8 on mavogel:chore/add-linter into a155912 on looplab:main.

Copy link
Member

@maxekman maxekman left a comment

Choose a reason for hiding this comment

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

Great suggestions with linting! I added some comments/questions.

.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
.golangci.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@maxekman
Copy link
Member

maxekman commented Sep 5, 2021

@mavogel Did you have time to read my comments?

@mavogel
Copy link
Contributor Author

mavogel commented Sep 6, 2021

Yes, @maxekman. I'll pick it up this week :)

@mavogel
Copy link
Contributor Author

mavogel commented Oct 17, 2021

Feel free to resolve the conversations and let discuss how we continue. As my terminal cannot capture all errors, I stored them temporarily into lint.txt go give an overview. I'll delete it later. Most probably it makes sense to deactivate linters such as

  • gochecknoinits
  • godox
  • exhaustivestruct
  • and scopelint

at local points for not changing the code too much in the first iteration. And from then on remove step by step those linters in future PRs. WDYT?

@maxekman
Copy link
Member

Nice work! 👌

I’ll go through the errors in the coming week and try to guess what level of linting/ignores is appropriate for the project.

Copy link
Member

@maxekman maxekman left a comment

Choose a reason for hiding this comment

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

I addad some notes on step naming to be changed be fore merging. I still need to go through the linting errors and decide what to fix etc.

.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
@maxekman
Copy link
Member

Would it be possible for you to separate out the Makefile command for linting so that appropriate linting settings and fixing linting errors can be done before activating the CI linting?

Makefile Show resolved Hide resolved
@mavogel mavogel marked this pull request as ready for review October 21, 2021 11:38
Copy link
Member

@maxekman maxekman left a comment

Choose a reason for hiding this comment

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

Very useful! 👍

@maxekman maxekman merged commit e17164b into looplab:main Oct 21, 2021
@mavogel mavogel deleted the chore/add-linter branch October 21, 2021 11:46
@maxekman
Copy link
Member

maxekman commented Oct 22, 2021

@mavogel The project will be ready to use the CI config for the linter after #349 and #348 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants