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

Set up git pre-commit hook and format the code automatically #1108

Closed
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@kt3k
Copy link
Contributor

kt3k commented Oct 27, 2018

Related to #363 (but not a direct solution)

This PR sets up .git/hooks/pre-commit from //tools/setup.py and it runs //tools/format.py at git commit and adds back the change to the staging of commit. With this change, committed files should be always formatted and it prevents unformatted codes to appear in future pull requests.

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Oct 27, 2018

@kt3k it needs to work on Windows too, right? Currently it doesn't.

@kt3k kt3k force-pushed the kt3k:feature/pre-commit-hook branch from 2fe0945 to 3e0e1cd Oct 27, 2018

[ -z "$staged" ] && exit 0
echo Formatting files
./tools/format.py
echo "$staged" | xargs git add

This comment has been minimized.

@piscisaureus

piscisaureus Oct 27, 2018

Collaborator

I don't want it to call "git add", because I very often don't commit all changes.

For this to be acceptable you need to run the contents from the index through the formatter, not the work tree file.

This comment has been minimized.

@kt3k

kt3k Oct 27, 2018

Author Contributor

This script formats all files, but it only adds staged files (because $staged is created from git diff --cached ... which only includes staged files.)

This comment has been minimized.

@kt3k

kt3k Oct 27, 2018

Author Contributor

I checked that this doesn't include unstaged changes (at least on my mac).

This comment has been minimized.

@hayd

hayd Oct 27, 2018

Contributor

@kt3k what if you only add some lines i.e. git add -p

This comment has been minimized.

@piscisaureus

piscisaureus Oct 27, 2018

Collaborator

What @hayd said is what I meant.
I do that all the time.

This comment has been minimized.

@kt3k

kt3k Oct 28, 2018

Author Contributor

I didn't think of that case enough... I'll try to think about the solution.

BTW lint-staged authors seem trying to solve a similar problem. ref. okonet/lint-staged#75
We might use the above tool when they shipped the above change?

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Oct 27, 2018

I've been trying to build on windows, but I'm seeing WindowsError: [Error 1314] from symlink function in util.py...

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

piscisaureus commented Oct 27, 2018

@kt3k
On windows 10 -> enable developer mode
Older windows -> https://superuser.com/questions/104845/permission-to-make-symbolic-links-in-windows-7

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Oct 28, 2018

@piscisaureus
developer mode worked and setup.py passed on my windows 10. Thanks!

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Oct 28, 2018

@kitsonk
I tested on PowerShell env and emulated bash env (on win 10). And the pre-commit hook itself seems working on both environments (though the partial staging problem still remains):

screen0

screen

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 28, 2018

I wonder if solution is to refactor lint.py to exit with status 1 if it made changes. And if lint makes changes in git commit hook then reject commit with a message (rather than git add).

@kt3k

This comment has been minimized.

Copy link
Contributor Author

kt3k commented Oct 29, 2018

Thanks for the suggestions and reviews. I understand rejecting commit is safer than fixing automatically, but I'm afraid it could be very stressful for contributors because of the loss of commit messages.

Now I think this topic is too premature. I'm closing for now. Sorry for bothering..

@kt3k kt3k closed this Oct 29, 2018

@kt3k kt3k deleted the kt3k:feature/pre-commit-hook branch Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment