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 autoformatting for Bazel files #2577

Merged
merged 4 commits into from
Aug 21, 2024
Merged

Add autoformatting for Bazel files #2577

merged 4 commits into from
Aug 21, 2024

Conversation

npaun
Copy link
Member

@npaun npaun commented Aug 21, 2024

No description provided.

@npaun npaun requested review from a team as code owners August 21, 2024 16:33
@npaun npaun linked an issue Aug 21, 2024 that may be closed by this pull request
tools/cross/format.py Outdated Show resolved Hide resolved
@npaun npaun force-pushed the npaun/add-bazel-formatting branch from 0117eed to b3f78e3 Compare August 21, 2024 16:54
@npaun npaun requested a review from danlapid August 21, 2024 17:00
@npaun npaun force-pushed the npaun/add-bazel-formatting branch from ddd9522 to e9909ed Compare August 21, 2024 17:10
@danlapid danlapid requested a review from fhanau August 21, 2024 17:16
@danlapid
Copy link
Contributor

I think @fhanau approval on this is a condition for merging

@fhanau
Copy link
Collaborator

fhanau commented Aug 21, 2024

See internal PR 8342 for some prior work on enabling buildifier automatically on the downstream repo – I was planning to patch up that PR today but looks like you got here first. It uses a different approach to downloading buildifier that I think is preferable, but I'll have to incorporate Dan's suggestions on how to invoke it – will provide some detailed notes on that later today and review this.

@danlapid
Copy link
Contributor

See internal PR 8342 for some prior work on enabling buildifier automatically on the downstream repo – I was planning to patch up that PR today but looks like you got here first. It uses a different approach to downloading buildifier that I think is preferable, but I'll have to incorporate Dan's suggestions on how to invoke it – will provide some detailed notes on that later today and review this.

Seems to me like we mind as well land this change as is and you can convert the installation method in a separate PR later.

Copy link
Contributor

@danlapid danlapid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependant on @fhanau approval of bazel file changes.

tools/cross/format.py Outdated Show resolved Hide resolved
tools/cross/format.py Outdated Show resolved Hide resolved
@fhanau
Copy link
Collaborator

fhanau commented Aug 21, 2024

This also applies for the other linters we added be we should document that buildifier needs to be installed as running the formatter is now a key part of development, even if invoked indirectly.

@npaun npaun force-pushed the npaun/add-bazel-formatting branch from e9909ed to 1948619 Compare August 21, 2024 19:00
Copy link
Collaborator

@fhanau fhanau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for this not being documented, but that goes for the other linters too.

@npaun npaun force-pushed the npaun/add-bazel-formatting branch from 391abde to 86b225a Compare August 21, 2024 19:16
@npaun
Copy link
Member Author

npaun commented Aug 21, 2024

I added a new issue to document the linters: #2581

@npaun npaun merged commit beb8566 into main Aug 21, 2024
10 checks passed
@npaun npaun deleted the npaun/add-bazel-formatting branch August 21, 2024 19:23
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 this pull request may close these issues.

Code formatting: Bazel
3 participants