-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
dev: group linter implementation and integration tests #4603
Conversation
Is it necessary to rename the |
Yes because the target will not run the linter tests.
The "new" make target is the same as before but the linter tests are not run by this target. Then it's not related to benefits or not, it's just the way it works.
It's used only one time and it's fixed inside this PR. |
8ce8bf6
to
f486fc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good work, I bet it was tedious so really appreciated! Interesting PR btw, never had this much issues rendering the GitHub diff view 😄
Just a question and dedup suggestion but LGTM! ✅
The goal is to group linter implementation and integration tests to facilitate the maintenance and the creation of linters.
It's the continuation of #3100 and #4578.
Each linter has a dedicated package and a dedicated
testdata
directory.It allows running all the tests for one linter easily without needing the
make test_linters
target.Also, the package
test
will now contain only "global" integration tests.The target
make test_linters
has been renamed totest_integration
.The configuration structures of linters are still inside the
config
package because of cyclic imports.It was not my priority for now because it's already a lot of changes.
The file
builder_linter.go
is still the center of the linter definition, it's intentional: behind the technical problems (cyclic imports) I think it's a good thing to have a central place to reference and document all the linters.This approach will not be applied to other pieces (e.g. processors, printers, etc.).
typecheck
andnolint/nolintlint
are special cases due to their nature (not real linters).Despite the apparent simplicity of this change, it takes me a lot of time to find a working approach, ensure there is no regression inside the tests, and do files move. (I redid my modifications dozens of times)
One commit, one change.