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

Script to create the Git pre-commit hook. #4622

Merged
merged 2 commits into from Feb 19, 2018

Conversation

4 participants
@tiagohillebrandt
Contributor

tiagohillebrandt commented Jan 19, 2018

Creates a Git pre-commit file to run phpcs once every commit (#4474).

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Jan 19, 2018

Contributor

Thanks for the PR @tiagohillebrandt! Although marked for the 1.5.0 milestone, this issue isn't a priority at the moment so it may take a little while before your PR is reviewed.

Contributor

gitlost commented Jan 19, 2018

Thanks for the PR @tiagohillebrandt! Although marked for the 1.5.0 milestone, this issue isn't a priority at the moment so it may take a little while before your PR is reviewed.

@danielbachhuber

👍 Looks reasonable to me. I'd like to get a second pair of eyes though.

@danielbachhuber danielbachhuber requested a review from wp-cli/committers Feb 12, 2018

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Feb 14, 2018

Member

The immediate problem I see is that this always runs PHPCS over the entire codebase, not over the commit that was staged.

It would be preferable to only check the files or even only lines that are being modified, otherwise you might not be able to commit something due to unrelated problems.

Member

schlessera commented Feb 14, 2018

The immediate problem I see is that this always runs PHPCS over the entire codebase, not over the commit that was staged.

It would be preferable to only check the files or even only lines that are being modified, otherwise you might not be able to commit something due to unrelated problems.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Feb 15, 2018

Member

The immediate problem I see is that this always runs PHPCS over the entire codebase, not over the commit that was staged.

Isn't this what our existing code does? I don't think it's an optimization we need right now...

Member

danielbachhuber commented Feb 15, 2018

The immediate problem I see is that this always runs PHPCS over the entire codebase, not over the commit that was staged.

Isn't this what our existing code does? I don't think it's an optimization we need right now...

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Feb 19, 2018

Member

Yes, you're right. Let's just go with this and try how it plays out. There's nothing preventing us from changing it down the line in case it is problematic.

Member

schlessera commented Feb 19, 2018

Yes, you're right. Let's just go with this and try how it plays out. There's nothing preventing us from changing it down the line in case it is problematic.

@schlessera schlessera merged commit 16a7d42 into wp-cli:master Feb 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera added this to the 2.0.0 milestone Feb 19, 2018

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Feb 19, 2018

Member

Hmm, already regretting to have merged this with full check:

image 2018-02-19 at 8 54 57 pm

Takes 25.5 seconds on my system to produce an empty commit.

Member

schlessera commented Feb 19, 2018

Hmm, already regretting to have merged this with full check:

image 2018-02-19 at 8 54 57 pm

Takes 25.5 seconds on my system to produce an empty commit.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Feb 19, 2018

Member

Takes 25.5 seconds on my system to produce an empty commit.

Isn't this better than waiting for Travis to build, and then having to come back ground to it?

Member

danielbachhuber commented Feb 19, 2018

Takes 25.5 seconds on my system to produce an empty commit.

Isn't this better than waiting for Travis to build, and then having to come back ground to it?

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Feb 19, 2018

Member

It just takes too much time. I'll add another PR to change this so it only checks files that were changed.

Member

schlessera commented Feb 19, 2018

It just takes too much time. I'll add another PR to change this so it only checks files that were changed.

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Feb 19, 2018

Contributor

Isn't this better than waiting for Travis to build, and then having to come back ground to it?

No!

Contributor

gitlost commented Feb 19, 2018

Isn't this better than waiting for Travis to build, and then having to come back ground to it?

No!

schlessera added a commit that referenced this pull request Feb 19, 2018

Only check staged files during the pre-commit PHPCS verification
This avoids causing unnecessary delays when committing files.

See #4622 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment