Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buildifier shouldn't break up blocks of constants with newlines #108

Open
jmillikin-stripe opened this issue Jun 9, 2017 · 8 comments · May be fixed by #1242
Open

Buildifier shouldn't break up blocks of constants with newlines #108

jmillikin-stripe opened this issue Jun 9, 2017 · 8 comments · May be fixed by #1242

Comments

@jmillikin-stripe
Copy link
Contributor

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
Copy link

alecbz commented Jun 13, 2017

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
Copy link
Contributor

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
Copy link
Member

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
Copy link

alecbz commented Jun 14, 2017

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?

mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jun 20, 2017
Add blank line as insisted upon by buildifier. This splits up a block of
related variables, disassociating one from its corresponding comment,
but there is no currently known way to stop this, and we need to be
buildifier-clean in order to switch on enforcing of style correctness as
judged by buildifier.

See also bazelbuild/buildtools#108.
mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jun 21, 2017
Add blank line as insisted upon by buildifier. This splits up a block of
related variables, disassociating one from its corresponding comment,
but there is no currently known way to stop this, and we need to be
buildifier-clean in order to switch on enforcing of style correctness as
judged by buildifier.

See also bazelbuild/buildtools#108.
@psigen
Copy link

psigen commented Oct 31, 2020

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:
#806 (comment)

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
Copy link
Member

vladmos commented Nov 2, 2020

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
Copy link

psigen commented Nov 3, 2020

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
Copy link
Member

vladmos commented Nov 3, 2020

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/"

CauhxMilloy added a commit to CauhxMilloy/buildtools that referenced this issue Feb 13, 2024
* Adding `CompactConstantDefinitions` boolean flag to `tables/tables.go`.
  * Can be set via tables json file (`--add_tables` / `--tables` command line or `tables` / `addTables` json config option).
* Updating `compactStmt()` in `build/print.go`.
  * If `CompactConstantDefinitions` is true and both statements are assignments (for const definitions), then the extra line should be removed.
  * Lower precedence than various other checks (like comment checks) to still allow extra lines when necessary.
* Updating `setFlags()` in `build/print_test.go`.
  * Checking golden filenames for ".compactconst." to set `CompactConstantDefinitions` to true.
  * Adding `CompactConstantDefinitions` reset back to false in returned (deferred) func.
* Copying various golden files (which contain constant definitions) to be ".compactconst." files.
  * Removing extra lines between const definitions as applicable.

Fixes bazelbuild#108.
@CauhxMilloy CauhxMilloy linked a pull request Feb 13, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants