Integrate linting #529
Integrate linting #529
Conversation
The `kotlin_repositories` macro is designed to download the third party dependencies that rules_kotlin has for itself. Because of the way that Bazel works, these must first be downloaded before they can be referred to by build files.
If a user has chosen to enable linting, `rules_kotlin` will now generate ktlint tests automatically. These can be run with a `bazel test --test_tag_filters=lint //...`
shs96c
requested review from
cgruber,
jin,
restingbull and
timpeut
as code owners
|
Note that this breaks the generated documentation - Stardoc doesn't follow the macro indirection. |
|
That's suboptimal. I'll fix that now. |
Stardoc doesn't allow a macro to wrap a rule and also have the rule's documentation be generated. Ideally, we'd simply exclude the lint wrappers and omit the `input` attribute from the `stardoc` target. However, thanks to [an issue with stardoc](bazelbuild/stardoc#99) this is not an option. As an alternative, we provide a verion of `rules.bzl` that deliberately excludes the lint wrappers.
|
I've fixed the problem with docs generation, but I've had to work around an issue in stardoc. |
|
That workaround is the same thing we do in rules_nodejs. Expose the rule to stardoc and the macro to users |
|
So does this remove the need to manually define kt_lint tests? They get autogenerated now ? |
|
This seems like something that should be opt-in so that kt-lint targets aren't being put on the golden path of repositories that don't rely on it. |
|
Note that as I commented on 83224e4#r49787303 (which I don't think anyone noticed as I commented on the commit after it was merged), the logic currently used to implement |
Yes, if you opt in they get autogenerated.
Docs in the PR say "This functionality is opt-in", is there a way we could make this more clear? |
|
I see that @gibfahn has beaten me to answering the two most pressing questions. Since the version of ktlint is pinned in this repo, I think it'd be okay to update it separately to this, but if people feel it's a blocker, I can explore the work that needs to be done and create a separate PR for that, since the |
Maybe opening an issue to track this and adding some documentation about this being a known issue and a link to it would be a good compromise? |
|
That's a great idea. I'm completely onboard with making |
cgruber
added
component: common
status: open - needs triage
type: cleanup
This builds on top of #528 in order to integrate lint tests. The second commit ("Automatically generate lint tests...") has more information in how to use the generated lint tests, and integrates the feature into the "anvil" demo.