Skip to content

Commit

Permalink
code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
alexeagle committed Oct 14, 2023
1 parent adc23e6 commit befb8f0
Show file tree
Hide file tree
Showing 7 changed files with 17 additions and 72 deletions.
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ This makes the build fail when any lint violations are present.

### 4. Failures during `bazel test`

Define a test rule `[linter]_test` using the [assert_no_lint_warnings](./docs/assert_no_lint_warnings.md) factory function.
Define a test rule `[linter]_test` using the [make_lint_test](./docs/lint_test.md) factory function.

Then add `[linter]_test` targets to your BUILD files, or teach your Gazelle extension to do this automatically.

If you prefer, you could expose this as a wrapper macro so that every
`foo_library` is a macro that expands to a pair of (`foo_library`, `foo_lint_test`) targets.
`[lang]_library` is a macro that expands to a pair of (`[lang_library`, `[linter]_test`) targets.

### 5. Code review comments

Expand Down
4 changes: 2 additions & 2 deletions docs/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
load("@aspect_bazel_lib//lib:docs.bzl", "stardoc_with_diff_test", "update_docs")

stardoc_with_diff_test(
name = "assert_no_lint_warnings",
bzl_library_target = "//lint:assert_no_lint_warnings",
name = "lint_test",
bzl_library_target = "//lint:lint_test",
)

stardoc_with_diff_test(
Expand Down
57 changes: 0 additions & 57 deletions docs/assert_no_lint_warnings.md

This file was deleted.

6 changes: 3 additions & 3 deletions example/lint.bzl
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
"Define linter aspects"

load("@aspect_rules_lint//lint:assert_no_lint_warnings.bzl", "assert_no_lint_warnings")
load("@aspect_rules_lint//lint:buf.bzl", "buf_lint_aspect")
load("@aspect_rules_lint//lint:eslint.bzl", "eslint_aspect")
load("@aspect_rules_lint//lint:flake8.bzl", "flake8_aspect")
load("@aspect_rules_lint//lint:lint_test.bzl", "make_lint_test")
load("@aspect_rules_lint//lint:pmd.bzl", "pmd_aspect")

buf = buf_lint_aspect(
Expand All @@ -20,11 +20,11 @@ flake8 = flake8_aspect(
config = "@@//:.flake8",
)

flake8_test = assert_no_lint_warnings(aspect = flake8)
flake8_test = make_lint_test(aspect = flake8)

pmd = pmd_aspect(
binary = "@@//:pmd",
rulesets = ["@@//:pmd.xml"],
)

pmd_test = assert_no_lint_warnings(aspect = pmd)
pmd_test = make_lint_test(aspect = pmd)
6 changes: 3 additions & 3 deletions lint/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
load("@bazel_skylib//:bzl_library.bzl", "bzl_library")

exports_files(glob(["*.bzl"]) + ["assert_no_lint_warnings.sh"])
exports_files(glob(["*.bzl"]) + ["lint_test.sh"])

bzl_library(
name = "buf",
Expand All @@ -20,8 +20,8 @@ bzl_library(
)

bzl_library(
name = "assert_no_lint_warnings",
srcs = ["assert_no_lint_warnings.bzl"],
name = "lint_test",
srcs = ["lint_test.bzl"],
visibility = ["//visibility:public"],
deps = ["@aspect_bazel_lib//lib:paths"],
)
Expand Down
12 changes: 7 additions & 5 deletions lint/assert_no_lint_warnings.bzl → lint/lint_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ To use this, in your `lint.bzl` where you define the aspect, just create a test
For example, with `flake8`:
```starlark
load("@aspect_rules_lint//lint:assert_no_lint_warnings.bzl", "assert_no_lint_warnings")
load("@aspect_rules_lint//lint:lint_test.bzl", "make_lint_test")
load("@aspect_rules_lint//lint:flake8.bzl", "flake8_aspect")
flake8 = flake8_aspect(
binary = "@@//:flake8",
config = "@@//:.flake8",
)
flake8_test = assert_no_lint_warnings(aspect = flake8)
flake8_test = make_lint_test(aspect = flake8)
```
Now in your BUILD files you can add a test:
Expand Down Expand Up @@ -43,7 +43,7 @@ def _test_impl(ctx):
for report in src[OutputGroupInfo].report.to_list():
reports.append(report)

bin = ctx.actions.declare_file("assert_no_lint_warnings.sh")
bin = ctx.actions.declare_file("lint_test.sh")
ctx.actions.expand_template(
template = ctx.file._bin,
output = bin,
Expand All @@ -55,13 +55,15 @@ def _test_impl(ctx):
runfiles = ctx.runfiles(reports + [ctx.file._runfiles_lib]),
)]

def assert_no_lint_warnings(aspect):
def make_lint_test(aspect):
return rule(
implementation = _test_impl,
attrs = {
"srcs": attr.label_list(doc = "*_library targets", aspects = [aspect]),
# Note, we don't use this in the test, but the user passes an aspect that has this aspect_attribute,
# and that requires that we list it here as well.
"fail_on_violation": attr.bool(),
"_bin": attr.label(default = ":assert_no_lint_warnings.sh", allow_single_file = True, executable = True, cfg = "exec"),
"_bin": attr.label(default = ":lint_test.sh", allow_single_file = True, executable = True, cfg = "exec"),
"_runfiles_lib": attr.label(default = "@bazel_tools//tools/bash/runfiles", allow_single_file = True),
},
test = True,
Expand Down
File renamed without changes.

0 comments on commit befb8f0

Please sign in to comment.