[FR]: Allow passing arguments to formatters in format_multirun

[momilo]

What is the current behavior?

At the moment it is not possible to provide arguments into the formatters specified in format_multirun. Instead, in order to configure their behaviour, one needs to rely on each formatter's internal (default) auto-discovery of configuration files (e.g. .editorconfig for shfmt, .yamlfmt for yamlfmt etc.) or rules_lint's supported .gitattributes annotations (to exclude certain paths - although, in this case, this applies to all formatters, not a specific one).

Describe the feature

It would be great to able to pass arguments to each individual formatter, to make full use of its features.

This would have a number of benefits:

• generally - not every formatter which supports CLI-based params supports also config files
• avoiding pollution of the repository root with config files - especially for monorepos with multiple languages, this quickly becomes quite an annoyance
• better control over what is included/excluded - some formatters include CLI-based support for what files to include/exclude - this differs between formatters/languages and allows for more precise operation - vs. the currently-available global attribute in .gitattributes (applied indiscriminately to all formatters)

[alexeagle]

@momilo could you point to some specific motivating examples of cases where a linter takes a CLI flag that you can't put in a config file?

pollution of the repository root with config files

rules_lint doesn't require this. We only recommend this layout in the example because some tools expect that configurations are in a parent folder of the files being formatted. You're free to put the file wherever you like if the formatter permits.

control over what is included/excluded

Again, I'd like to see a motivating example of a case where a file should be excluded for one formatter but not another. Wouldn't it be a bad experience if two different formatters are trying to operate on the same file?

Thanks @ewhauser for pointing out an existing workaround: put these in your own wrapper around the binary, then register that one:

sh_binary(
     name = "format-sql",
     srcs = [
         "format-sql.sh",
     ],
     data = [
         "//:pyproject.toml",
         "//tools/sql-formatter",
         "@pypi_shandy_sqlfmt//:rules_python_wheel_entry_point_sqlfmt",
     ],
     visibility = ["//visibility:public"],
     deps = [
         "@bazel_tools//tools/bash/runfiles",
     ],
 )
 format_multirun(
     name = "format",
     sql = ":format-sql",
 )

[momilo]

Hey Alex,
Thank you for the response.

I think my argument is more focused around "if I don't absolutely need to use many additional files for trivial things, I would rather not". This wouldn't be an issue in a small repo. But running a monorepo with many "languages", creating a number of files e.g. just to specify the number of space indentations (shfmt, looking at you...) seems... like an overkill.

But, to be more specific, answers below.

could you point to some specific motivating examples of cases where a linter takes a CLI flag that you can't put in a config file?

That is not what I meant. I think it might be an incorrect assumption that each and every formatter necessarily must support a config file. From the top of my head, for instance, formatters which don't support config files:

• gofumpt - you can pass -extra argument to enable additional rules
• tanka fmt (used for formatting tanka, Grafana authored, jsonnet-based tool for kubernetes manifests generation) (sidenote: I was hoping to add it to rules_lint, if there is interest, as an alternative for jsonnet, similarly to both fmt and gofumpt being supported for Go) - CLI used mostly to pass paths/exclusions etc. (fair, that's a weaker argument)

I wouldn't be surprised if many other formatters (perhaps contrary to linters, which usually are usually much more complex) do not support config files.

pollution of the repository root with config files
rules_lint doesn't require this. (...) You're free to put the file wherever you like if the formatter permits.

Ish. Usually the formatters, if they accept a config file, take the approach of "the config file auto-discovered will be used for directories underneath". In monorepos this forces you to put them in the root (or copy-paste/symlink across a number or dirs). If you have access to CLI... you can often just hide the mess, like many usually do e.g. with golangci-lint or yamlfmt (chuck the configs into /build/golangci.yaml or a similar /config/..., pass the flag to the config on invocation, but run it against a different set of directories).

Again, I'd like to see a motivating example of a case where a file should be excluded for one formatter but not another.

That's a fair point. I was probably thinking more about including only a language-specific codepath (e.g. apps/go/ for Go formatting, apps/js/ for javascript etc.) but... perhaps running everything against the whole world doesn't hurt, since it's filetype driven anyway.

Overall - it's not a big thing (I would rather cheer for superfast golangci-lint support :-D), especially since now I have learnt of the helpful workaround. Just something to potentially consider.

[TimotheusBachinger]

Not completely sure if my question is related (but it feels like):
In https://github.com/aspect-build/rules_lint/blob/5b004ffa9d02c113a1f0ffb8bca69db8eed0f4ac/format/private/formatter_binary.bzl#L52C5-L52C41 you're defining command line arguments for the tools running in check mode.
terraform-fmt for example has the -diff option set. I want to set this option as well for ruff to give a hint what needs to be re-formatted. Am I missing the API to do that?

[alexeagle]

@TimotheusBachinger I think that's a different issue. If you run in check mode I expect ruff and terraform-fmt to behave similarly: print the difference of what should be changed. No need for the user to pass arguments to the formatters. If that's not the case, could you file a separate issue please?

[alexeagle]

@momilo would this be sufficient?

format_multirun(
    name = "format",
    go = "@aspect_rules_lint//format:gofumpt",
    go_args = "-extra",
)

That's semantically equivalent to the workaround above, but nicer syntax sugar and doesn't need a bash wrapper. It still defines a formatter that uses fixed arguments regardless of what directories contain the files it formats.

[TimotheusBachinger]

@alexeagle I have simply created a PR to achieve what I want:
#386

[momilo]

@momilo would this be sufficient?

Totally. I ended up doing "$YAMLFMT_BIN" $CUSTOM_PARAMS "$@" in the bash script, which seems to be the exact same thing (add additional params, then let multirun provide the rest / required ones).

[mattnworb]

• avoiding pollution of the repository root with config files - especially for monorepos with multiple languages, this quickly becomes quite an annoyance

+1, we currently patch rules_lint with something like

-    "yamlfmt": "-lint",
+    "yamlfmt": "-conf tools/yamlfmt.yaml -lint",

just to set where the config file is stored. Creating a wrapper for this is feasible enough but it adds a few dozen lines of code (which feels more cumbersome than a one line .patch file, to be honest).

[alexeagle]

@mattnworb why do you need to do that though? According to https://github.com/google/yamlfmt/blob/main/docs/config-file.md#config-file-discovery it should locate the config file automatically. Is it finding the wrong file?

[mattnworb]

sorry for not being clear - we had a (very subjective) desire to put the config file in some place other than the root of the repo / Bazel workspace, in which case yamlfmt seems to require the -conf tools/yamlfmt.yaml flag for example when formatting files in the foo/bar subdirectory.

[alexeagle]

Okay, I think "we want to do something the tool doesn't support" is fine but means the complexity of supporting that should fall on you. I'm trying to be very parsimonious in what we add to rules_lint so that the required maintenance budget stays feasible.

So my assessment is that yamlfmt example isn't enough motivation to add this feature. Maybe there are still other good ones.

[elwin]

Just throwing into the mix: I believektfmt is also only configurable through CLI flags (well, there's just two of them)

[Undo1]

I'm very new to Bazel, but trying to use this to run clang-format. It's running, but not actually formatting any files because it's not running @llvm_toolchain_llvm//:bin/clang-format with the -i flag. I think the output is going to stdout and getting lost.

Am I right to think that this is required for that use case, or (more likely) am I missing something simple?

[alexeagle]

@Undo1 no that ought to just work. You should not need to pass any flags to it.