buildtools

buildtools copied to clipboard

Does Buildifier support reading the lint rules from a configuration file? Something like Prettier or jshint do...

The use case is that we need to run Buildifier with a subset of lint rules and passing all of the rules in the command line is just not convenient.

at the moment the list of warnings is configured only via warnings flag

Are you folks open for such a feature?

+1 for this feature - look at the configuration we use for Angular: https://github.com/angular/angular/blob/cfe6581fc8c292dc7aa1aac07a461de64ebf1c36/package.json#L19 the command line is unreadable because we select such a large number of rules

Can you clarify how it would work? It's a new flag on the command-line to load flags from a file?

The content of the file is something like this?

-v --warnings=args-order,attr-cfg,attr-license,attr-non-empty

I think buildifier should have an .rc file at a standard location, like .bazelignore and .bazelrc. I don't care if there's a command-line arg to override the location of that file. I'll call it .bazellintrc below, which I think is nice since it alpha-sorts in my project next to other Bazel configs, and the "buildifier" codename doesn't convey as much meaning as the function it performs.

Overall we want:

• some directories don't contain our sources and should be ignored. Entries in .bazelignore should probably be ignored by Buildifier. Maybe additional ignore paths could be specified in the .bazellintrc file
• it would cause fewer merge conflicts if the enabled warnings were on separate lines
• if I run buildifier with no args I'd like it to pick up the configuration checked into this file

@laurentlb I was thinking of taking a crack at this... Are you open to reviewing a PR for it?

I was thinking of:

• Scanning up the cwd to find a .buildifierrc file
• Adding a --buildifierrc=FILE flag to override the location of the file

Format of file would be:

• ok to have multiple flags on same line (e.g. -v --warnings=args-order,attr-cfg,attr-license,attr-non-empty)
• newline separator is ok, so -v\n--warnings=args-order,attr-cfg would have same effect
• continuation characters is ok, so -v\n--warnings= \\nargs-order \nattr-cfg is also valid
• ^\s*#.*$ is a comment

WDYT?

The idea looks reasonable to me, but I'll let @vladmos review the design (and the PR).

I very much support @pcj idea. We use buildifier in CI to enforce the formatting and also use it interactively in VSCode. Without a feature like that, it is close to impossible to keep CI and interactive settings in sync.

This looks reasonable to me. Two things:

• With the (current) command line interface warnings won't work unless you provide --lint=warn (to show the warnings) or --lint=fix (to fix if possible), I don't thing that .buildifierrc should implicitly add any of them if --warnings are provided because that can be confusing, on the other hand I don't think buildifier should fail if no--lint flag is provided but there's a .buildifierrc file with warnings. I'm not sure what's the best solution, maybe the --lint flag should be also a part of the file to make the whole interface consistent, i.e. --warnings without --lint will still cause an error.

• Please keep in mind that the arguments for --warnings may be prepended with + or - with semantics "all default warnings +warning1 - warning2" (usage example: https://github.com/bazelbuild/buildtools/blob/master/buildifier/integration_test.sh#L197). It's only allowed to have either all warnings with + or - or none of them. That's implemented in https://github.com/bazelbuild/buildtools/blob/master/buildifier/utils/flags.go#L102, I think it would be nice to just reuse the function to parse the warnings uniformly.

Can we disable single warning and have all other to validate? In the below currently there is no option to include all the warning except one so do we have other way to do this?

buildifier --lint=warn --mode=check --warnings=-native-android,+all 

@anandwana001 do you want to enable all the warnings, even those that are disabled by default, except one?

@pcj proposal above is good, but what do you think we should do for the include/exclude paths behavior? Right now there is no agreement on include (e.g. bugfixes like https://github.com/jlebar/pre-commit-hooks/pull/9) and no one has exclude (but I'd want to ignore a third_party directory for example)

Hey any traction on this? This sort of thing would be super helpful to have to eliminate redundant flags when working with VSC

PR is available at https://github.com/bazelbuild/buildtools/pull/1080. LMK if you have any issues with the design.

@alexeagle include/exclude paths can be done outside buildifier with scripting, although there could be a rationale for putting that into buildifier as a feature. However, it should be a separate PR as it affects buildifier itself and would require new flags for that.