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 pre-commit hook for buildifier #1169

Merged
merged 2 commits into from
Jun 2, 2021
Merged

Conversation

alexeagle
Copy link
Contributor

My experience contributing here has been that nearly every PR snapshot required an extra push to run a buildifier command after a red CI run. This is easily automated.

@google-cla google-cla bot added the cla: yes label May 27, 2021
@keith
Copy link
Member

keith commented May 29, 2021

This is a great idea to have, for a while I had been meaning to bridge this to pre-commit automatically so you don't even have to worry about users already having the right version installed. I've just pushed a repo for this here https://github.com/keith/pre-commit-buildifier

@alexeagle
Copy link
Contributor Author

Sweet! Can we get the buildifier version aligned with .bazelci somehow? I worry a new release has a new lint rule that gets auto-enabled in one place but not the other

@keith
Copy link
Member

keith commented May 29, 2021

Afaiui bazelci uses "newest" even if that means it'll start to block you at any point. I guess the closest thing to that would be to point to main of the pre-commit plug-in, but pre-commit discourages that, and it would still require an update on that side. I still think using this would be closer to what we want since at least we can force users to upgrade when there is a new version as opposed to hoping everyone does manually locally

@alexeagle
Copy link
Contributor Author

Would it make sense for the bazelbuild/buildtools repo to be the one we install the pre-commit hook from? Then it is more obviously canonical rather than coming from your github org, and also means the rev field in the pre-commit config is exactly the same as the buildifier version you want

@alexeagle
Copy link
Contributor Author

oh and you can totally pin bazelci to the matching version like https://github.com/bazelbuild/rules_nodejs/blob/stable/.bazelci/presubmit.yml#L6

@keith
Copy link
Member

keith commented May 30, 2021

Would it make sense for the bazelbuild/buildtools repo to be the one we install the pre-commit hook from?

I think ideally so, but I didn't want to try to impose more maintenance on those folks unless this proves to be useful. I also don't know if they would like the same type of from binary hook as opposed to building from source.

and also means the rev field in the pre-commit config is exactly the same as the buildifier version you want

I actually intentionally made the version have an extra component so you could have a number to bump for hook-only fixes (I considered omitting it at .0 but felt like that would be weird too)

oh and you can totally pin bazelci to the matching version

Ah I thought you were suggesting pinning the hook version to "whatever bazel CI is" and not the other way around. I think for now it would be less work for us to leave the CI version as latest, and deal with the fallout (which ideally would also encourage us to bump the local hook version), as opposed to having to bump the CI version manually.

My experience contributing here has been that nearly every PR snapshot required an extra push to run a buildifier command after a red CI run. This is easily automated.
.bazelci/presubmit.yml Outdated Show resolved Hide resolved
@keith keith merged commit 61bc7c0 into bazelbuild:master Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants