Buildifier shouldn't break up blocks of constants with newlines

[jmillikin-stripe]

UBUNTU_MAIN = "http://mirrors.kernel.org/ubuntu/pool/main/"
UBUNTU_UNIVERSE = "http://mirrors.kernel.org/ubuntu/pool/universe/"
NODEJS = "http://deb.nodesource.com/node_6.x/pool/main/"

becomes

UBUNTU_MAIN = "http://mirrors.kernel.org/ubuntu/pool/main/"

UBUNTU_UNIVERSE = "http://mirrors.kernel.org/ubuntu/pool/universe/"

NODEJS = "http://deb.nodesource.com/node_6.x/pool/main/"

This is not ideal because a BUILD file with blocks of related constants can become very "fluffy" and difficult to scan over.

Copying the gofmt logic of aligning equals signs in blocks of assignments would be nicer:

UBUNTU_MAIN     = "http://mirrors.kernel.org/ubuntu/pool/main/"
UBUNTU_UNIVERSE = "http://mirrors.kernel.org/ubuntu/pool/universe/"
NODEJS          = "http://deb.nodesource.com/node_6.x/pool/main/"

Or, alternatively, just don't add newlines.

[alecbz]

Note that this rule is currently codified in the style guide:

Use a single blank line between two top-level definitions. Rationale: The structure of a BUILD file is not like a typical Python file. It has only top-level statements. Using a single-blank line makes BUILD files shorter.

Although the motivation seemed to be one line as opposed to two.

[mwoehlke-kitware]

First off, that rationale is wrong; we have quite a few Bazel files full of utility functions, rule implementations, etc. that most certainly do not "[have] only top-level statements".

That said, I agree the intent seems to be 'one blank line, not two', rather than 'one blank line, not none'. Anyway, here's another example where adding a blank line is clunky, if not outright misguided:

# This is a comment that applies to multiple declarations.
FOO = "foo"
BAR = "bar"

With no blank line, it is clear that the comment applies to both declarations. Adding a blank line here is actively harmful to clarity.

[pmbethe09]

This change does seem reasonable and worth considering, but note that internally it will require a depot-wide change so will have to be rolled out cautiously.

[alecbz]

First off, that rationale is wrong; we have quite a few Bazel files full of utility functions, rule implementations, etc. that most certainly do not "[have] only top-level statements".

Bazel files (.bzl) or BUILD files?

[psigen]

Is there any activity on fixing this? Or is there a way to disable it for a block of code (like clang-format: off?)

I have a project where I have many groups of related constants in my WORKSPACE (version numbers/hashes/urls for repositories), and this rule is making them extremely difficult to read.

I found this thread which suggests it's not possible: https://github.com/bazelbuild/buildtools/issues/806#issuecomment-628781060

I also looked through this list of warnings for anything I could disable but didn't immediately find anything relevant: https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#how-to-disable-warnings

[vladmos]

There's a recent discussion on it in #890, however there's currently no activity.

It might be possible to disable formatter for a specific node with a comment, but in the example above one would need to comment every assignment statement:

UBUNTU_MAIN     = "http://mirrors.kernel.org/ubuntu/pool/main/"  # buildifier: disable-format
UBUNTU_UNIVERSE = "http://mirrors.kernel.org/ubuntu/pool/universe/"  # buildifier: disable-format
NODEJS          = "http://deb.nodesource.com/node_6.x/pool/main/"  # buildifier: disable-format

Perhaps an easier approach would be to load all the necessary definitions from a .bzl file which is marked as "do not format" with a comment.

Regarding blank lines, they are not enforced anymore in .bzl files (there are few exceptions, but in general users can decide when to separate statements with blank lines).

[psigen]

Alright, I will try the workaround of moving all my blocks into separate files. I am not sure that will be too convenient either, as these were for repository imports that need to be interleaved with load commands, so it will be dozens of 5 line files, but it might be easier than putting disable-format on every line. 😞

Thank you for pointing out that this is specific to WORKSPACE files. That is very useful to know.

[vladmos]

Note that the disabling comment is not yet implemented, that was just an idea of how it could work.

If you mean forced empty lines between statements, that's specific to BUILD and WORKSPACE files, for .bzl files users have more control of how each statement should be formatted. You can move all your constants to a .bzl file and group the statements as you want, e.g.:

UBUNTU_MAIN = "http://mirrors.kernel.org/ubuntu/pool/main/"
UBUNTU_UNIVERSE = "http://mirrors.kernel.org/ubuntu/pool/universe/"

NODEJS = "http://deb.nodesource.com/node_6.x/pool/main/"