bazel-skylib
bazel-skylib copied to clipboard
bazelbuild
Build kite says failing because of lint errors
These appear to be mostly from analysis-time test suites, which are implemented as macros but do not (and should not) have a name parameter. We should have a way to disable this lint for these files, but it's also a more general problem for anyone using buildifier with this analysis testing paradigm.
So this is a least 3 distinct bugs. And yet it is not obvious if any are syklib's problem
- buildifier should have capabilities to have different requirements for rules vs. macros
- buildifier should have an option to disable a warning class with one structured comment at the file level (I'm not sure if it does not)
- the CI system should be able to have a no-lint option in the PR description to make the warnings just advisory and not failures
These are actually macros, not rules. The starlark test suites are really just macros that instantiate test cases (rules).
RIght. That's why I said different requirements for rules vs. macros. We can't have rule usage without names. We should be able to have macros without name.
I think the lint intends to prevent you from creating macros without a name argument, because it's usually considered bad form. Certainly that's something we're trying to discourage in google3. The problem is that this isn't really a "macro" in the usual sense, but is still subject to the same requirement.
One option aside from opting out of the lint is to modify our recommended testing practices to allow for a name argument passed to the test suite.
I don't really want to debate what buildifier should do here. That might be fun, but it's the wrong forum. For this, what shall we do? Force submit the CL now, or add features to buildifier or CI to make it easier to submit this without a force?
On Fri, May 8, 2020 at 11:15 PM Jon Brandvein [email protected] wrote:
I think the lint intends to prevent you from creating macros without a name argument, because it's usually considered bad form. Certainly that's something we're trying to discourage in google3. The problem is that this isn't really a "macro" in the usual sense, but is still subject to the same requirement.
One option aside from opting out of the lint is to modify our recommended testing practices to allow for a name argument passed to the test suite.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel-skylib/issues/247#issuecomment-626096579, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHAUPYKJ7CEAW2M2J4LRQTDD7ANCNFSM4M4HG37A .
Not sure which CL you mean, I noticed this issue independently of any PR. I'm fine with force-submitting past any lint-only blockers, though if that becomes the habit we should remove the lint from the presubmit.
SGTM. I keep using PR and CL interchangeably.
On Sat, May 9, 2020 at 3:17 PM Jon Brandvein [email protected] wrote:
This is already tracked upstream by bazelbuild/buildtools#821 https://github.com/bazelbuild/buildtools/issues/821.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel-skylib/issues/247#issuecomment-626223147, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHBMDBH2KI3PDYHRJYTRQWT47ANCNFSM4M4HG37A .