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

Add buildifier as a BUILD file formatter #16573

Merged
merged 4 commits into from
Aug 22, 2022

Conversation

thejcannon
Copy link
Member

This is the first plugin I actually question if it belongs here in the Pants repo, and not in another package. It's a bit of an abuse to run Bazel's Starlark formatter on Pants BUILD files, but:

  1. I've basically never seen a syntax error in the wild, since BUILD files are relatively declarative. Only one I've seen is Starlark doesn't allow implicit string concat. I kinda like that 😛
  2. Since we're looking towards Add new fmt plugins for build file formatting #16560 this is actually really useful for incremental adoption, where users who are using Pants for fomat+lint+check and Bazel for build+test and using two distinct files. We could fmt/lint their Bazel for them and show them how cool our caching is 😄 Until then, it's useful if you're doing the "two systems one file trick".

Additionally, this PR does not add any rules for running buildifier's lint capabilities as I don't think Pants would trigger those and not be a false positive. Once we implement #16560 it might be worthwhile.

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood
Copy link
Member

stuhood commented Aug 20, 2022

This is the first plugin I actually question if it belongs here in the Pants repo, and not in another package.

I'm fine with having it in repo. Until the plugin API is stable, a burden is imposed on external plugins as APIs change. Post API stabilization, we can figure out whether to externalize plugins en-masse.

@thejcannon
Copy link
Member Author

This is the first plugin I actually question if it belongs here in the Pants repo, and not in another package.

I'm fine with having it in repo. Until the plugin API is stable, a burden is imposed on external plugins as APIs change. Post API stabilization, we can figure out whether to externalize plugins en-masse.

I've thought about this more. I think the best middle-groud to all this is in-repo but out of pants-packaged. So dogfooding the monorepo-but-multi-dist support.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano
Copy link
Contributor

I think the best middle-groud to all this is in-repo but out of pants-packaged. So dogfooding the monorepo-but-multi-dist support.

That was the v1 approach to all plugins. I don't see a ton of benefit to this: the versioning of the plugin is closely coupled to pantsbuild.pants itself. As long as a backend doesn't add new requirements, then there seems to be little cost to adding to pantsbuild.pants (trivially bigger package size).

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon merged commit 1b3192a into pantsbuild:main Aug 22, 2022
@thejcannon thejcannon deleted the buildifier branch August 22, 2022 18:15
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants