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

feat(chore): add golangci-lint #311

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

darkweak
Copy link

@darkweak darkweak commented Apr 21, 2023

Here is the rebased PR with only needed changes @tucksaun

  • Only already checked errors are handled by the errors.WithStack wrapper
  • Others are just ignored
  • Only few linters are enabled at the moment but the list can be increased later in another PR

@darkweak darkweak force-pushed the feat/chore/add-golangci-lint branch 4 times, most recently from 922192f to 34f0f0c Compare April 21, 2023 16:10
@darkweak darkweak force-pushed the feat/chore/add-golangci-lint branch from 34f0f0c to 1ffee82 Compare April 21, 2023 16:19
@tucksaun
Copy link
Contributor

@darkweak did I miss something? the PR is now bigger than #245 😅

@darkweak
Copy link
Author

@tucksaun there are new returned errors now 😅

@darkweak
Copy link
Author

darkweak commented May 4, 2023

@tucksaun do you plan to review that? In fact that's just a search and replace to wrap errors.

@tucksaun
Copy link
Contributor

This is not as simple as a search and replace as some are not necessary or some could change the semantic of the error by changing its type and thus could break check based on it which is particularly risky given the fact that we miss tests on those.

Regarding your question, given the size of this PR and my current free time that I can allow to OSS I double I will have time soon enough to review this PR in a single iteration quickly enough so that there's not conflict and no need for back and forth. That's why I invited you to submit smaller scoped contributions in your original PR that way we can more easily assess the changes and merged them without blocking the whole contribution.

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.

None yet

2 participants