-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
dev: group linter implementation and integration tests #4603
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
Conversation
2153d9e to
04a3085
Compare
Is it necessary to rename the make target test_linters to test_integration? This target is frequently used in the documentation and in messages such as #3527 (comment). Could you please share the benefits of using the new make target name?
Is it necessary to rename the make target test_linters to test_integration?
Yes because the target will not run the linter tests.
Could you please share the benefits of using the new make target name?
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.
This target is frequently used in the documentation
It's used only one time and it's fixed inside this PR.
8ce8bf6 to
f486fc1
Compare
d8013f9 to
57ee65a
Compare
57ee65a to
400f3f7
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! ✅
Uh oh!
There was an error while loading. Please reload this page.
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
testdatadirectory.It allows running all the tests for one linter easily without needing the
make test_linterstarget.Also, the package
testwill now contain only "global" integration tests.The target
make test_lintershas been renamed totest_integration.The configuration structures of linters are still inside the
configpackage because of cyclic imports.It was not my priority for now because it's already a lot of changes.
The file
builder_linter.gois 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.).
typecheckandnolint/nolintlintare 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.