-
Notifications
You must be signed in to change notification settings - Fork 205
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
Add support for linters #1294
Comments
I guess we need some way to populate the module cache and share that between rules. Please doesn't use a module cache at the moment does it? Maybe we can use a file system based module proxy or something to speed up downloads. Might be an easier thing to populate. Sounds a bit fiddly. Alternatively, perhaps go lint can work without modules? I think it would be possible to get please to build up a traditional GOPATH based tree of all the sources. Perhaps it can lint based on that rather than having it download deps based on the go.mod. |
I think that would be make using modules much easier. Keep in mind though, that copying all dependencies wouldn't be an option as they could potentially be GBs.
That wouldn't work with modules not available in the GOPATH. I wouldn't rely on GOPATH for the long term, Go is moving away from it. |
I've got some experience with linting in Arcanist. The nice thing about arc is that you can configure a large number of linters to run and restrict them to certain files or parts of the repo. No matter how complex your repo gets, you can always lint all your changes with a simple Please is designed for large heterogeneous monorepos so it could be argued that linting could fall into Please's domain. I'd like more than just performance as a design goal however. What do you think of the following:
Unlike tests, users shouldn't have to define lint rules throughout their build graph per target. Lint rules should apply to all sources based on some basic include and exclude rules. Alternatively, we could update the built in rules to automatically generate lint targets e.g. go_library could generate a lint rule for all it's sources. I think this goes against goal #3 though. We want to be more flexible than this. For goal 2, Perhaps We have a build system; we can define custom binary rules that abide by some linting contract. If we can, we should find some common standard for linter input and output format. Github must have some format that allows lint actions to report lines and files for lint errors/warnings. Perhaps we can borrow that. Once we have a binary rule that abides by our format, we're going to want some way to tell Please about it:
However we might also want to restrict linters to certain file extensions or exclude directories. Perhaps we need a
Please can then pick up all the lint rules and run them against the relevant sources. The We can consider if the linters should have a persistent working directory within |
FYI you can also have more complex structures in the config, for example
|
For aggregating the results from multiple linters, I'd take a look at reviewdog: https://github.com/reviewdog/reviewdog#input-format. Go is once again going to make our lives harder: some linters use the goddamn |
This issue has been automatically marked as stale because it has not had any recent activity in the past 90 days. It will be closed if no further activity occurs. If you require additional support, please reply to this message. Thank you for your contributions. |
No stalebot, we're just thinking it over still. |
This issue has been automatically marked as stale because it has not had any recent activity in the past 90 days. It will be closed if no further activity occurs. If you require additional support, please reply to this message. Thank you for your contributions. |
Bump |
I wonder if you could combine the "tools" approach with hidden rules created by build/test rules that are tagged as a e.g. in .plzconfig
in pkg/go/mygolib/BUILD.plz
in pkg/python/mypylib/BUILD.plz
in pkg/python/myotherpylib/BUILD.plz
in infra/k8s/myenv/BUILD.plz
With the above, the user interface would basically be |
@numbsafari Yeah something like that would be pretty good, but only works for simple linters. I think that's essentially what Peter played around with here, but made it a bit more first class: More complex linters like |
This is just a random idea for now, but adding support for linters would be a nice feature.
In a large Go project, using linters currently requires populating the "standard" module cache which basically means downloading dependencies a second time (first time being when Please downloads them).
I can imagine creating a
go_lint
rule that runs linters for each package separately, but should it be a test?As @Tatskaari pointed out on Gitter, the results of a linter might differ from test results.
From my perspective, having to download dependencies a second time complicates things (build pipeline and environment) enough to make the case for using Please harder. 😕 On the other hand, I don't see how "lint support" would look like yet.
The text was updated successfully, but these errors were encountered: