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

Make buildifier run on changed files only, not all files #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

agasparovic-sabre
Copy link

Pre-Commit will pass the list of changed files that match the rule's pattern. This means the script isn't needed, and the rule can use the 'system' language.

It's not necessary, since "buildifier" can be configured in .pre-commit-hooks.yaml. Furthermore, it shouldn't be run on all files, just the changed ones.
Make buildifier use 'system' language - it doesn't need to be a script (since buildifier should be run on the changed files, not all files).
@agasparovic-sabre
Copy link
Author

Thanks a lot for creating this Pre-Commit rule for BUILD files!

@agasparovic-sabre
Copy link
Author

Hi @FelixSeptem, any time to review this? Happy to make changes as necessary!

@jlebar
Copy link

jlebar commented Feb 25, 2020

Another reason this is worthwhile is because your .git directory might contain files named BUILD.

@jlebar
Copy link

jlebar commented Feb 25, 2020

FWIW I put this up at https://github.com/camusenergy/pre-commit-hooks

Ideally I'd set up a Docker so that it automatically fetches buildifier, but that's beyond me for the moment.

@agasparovic-sabre
Copy link
Author

Thanks @jlebar!

@TheButlah
Copy link

I just submitted #3 which seems related to this. These two PRs complement one another, I'll pass on my PR to @jlebar's repo as well

@TheButlah
Copy link

Pinging @FelixSeptem to determine if this is planned on being merged

@TheButlah
Copy link

Submitted a duplicate of my fix in #3 to @jlebar's repo here: jlebar/pre-commit-hooks#1

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.

None yet

3 participants