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 pre-commit execute linter only if php files have been staged #12031

Merged
merged 4 commits into from Jan 8, 2019

Conversation

Projects
None yet
3 participants
@eternoendless
Copy link
Member

eternoendless commented Jan 4, 2019

Questions Answers
Branch? develop
Description? Currently, the pre-commit hook executes php-cs-fixer even if there are no modified php files. This PR makes it so it will only be executed if php files have been changed and staged for commit.
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? No QA test needed

This change is Reviewable

@eternoendless eternoendless added this to the 1.7.6.0 milestone Jan 4, 2019

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 7, 2019

The added test seems incompatible with /bin/sh

$ git commit -m "Nope"
.git/hooks/pre-commit: 18: .git/hooks/pre-commit: [[: not found
No modified PHP files to check.
[eternoendless-improve-pre-commit dc5dcceff1] Nope
 1 file changed, 1 insertion(+), 1 deletion(-)

Check must be updated

@eternoendless

This comment has been minimized.

Copy link
Member Author

eternoendless commented Jan 7, 2019

Fixed 👍

echo "Running PHP CS Fixer..."
$PHP_BINARY ./vendor/bin/php-cs-fixer fix
echo "Running PHP CS Fixer..."
$PHP_BINARY ./vendor/bin/php-cs-fixer fix --show-progress=dot

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 7, 2019

Member

This command always return 0. If you want to prevent a commit to be done if any file needs fixing, you must use the --dry-run parameter.

From the documentation:

  Exit codes
  ----------
  
  Exit code is built using following bit flags:
  
  *  0 OK.
  *  1 General error (or PHP minimal requirement not matched).
  *  4 Some files have invalid syntax (only in dry-run mode).
  *  8 Some files need fixing (only in dry-run mode).
  * 16 Configuration error of the application.
  * 32 Configuration error of a Fixer.
  * 64 Exception raised within the application.
  
  (applies to exit codes of the `fix` command only)
Suggested change Beta
$PHP_BINARY ./vendor/bin/php-cs-fixer fix --show-progress=dot
$PHP_BINARY ./vendor/bin/php-cs-fixer fix --show-progress=dot --dry-run

This comment has been minimized.

@eternoendless

eternoendless Jan 7, 2019

Author Member

I guess I should change it to --dry-run, then issue a message encouraging the developer to run /vendor/bin/php-cs-fixer fix to fix it, then commit again.

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 7, 2019

Member

Good idea, this is the objective of #11920 when done in Travis.

This comment has been minimized.

@eternoendless

eternoendless Jan 8, 2019

Author Member

Done!

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 8, 2019

Current result:

$ git commit -m "Nope"
Checking PHP Lint...
No syntax errors detected in /home/thomas/Documents/github/PrestaShop/src/PrestaShopBundle/Model/AdminModelAdapter.php
Running PHP CS Fixer...
Loaded config default from "/home/thomas/Documents/github/PrestaShop/.php_cs.dist".
Using cache file "/home/thomas/Documents/github/PrestaShop/var/.php_cs.cache".
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS   61 / 1721 (  4%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  122 / 1721 (  7%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  183 / 1721 ( 11%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  244 / 1721 ( 14%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  305 / 1721 ( 18%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  366 / 1721 ( 21%)
SSSSSSSSSSSSSSSSSSSSSSSSSSFSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  427 / 1721 ( 25%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  488 / 1721 ( 28%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  549 / 1721 ( 32%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  610 / 1721 ( 35%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  671 / 1721 ( 39%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  732 / 1721 ( 43%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  793 / 1721 ( 46%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  854 / 1721 ( 50%)
SSSSSSSSSSSSSSSSSSSSSSSSSSS.SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  915 / 1721 ( 53%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS  976 / 1721 ( 57%)
SSSSSSSSSSSSSSSS..SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1037 / 1721 ( 60%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1098 / 1721 ( 64%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1159 / 1721 ( 67%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1220 / 1721 ( 71%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1281 / 1721 ( 74%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1342 / 1721 ( 78%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1403 / 1721 ( 82%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1464 / 1721 ( 85%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1525 / 1721 ( 89%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1586 / 1721 ( 92%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1647 / 1721 ( 96%)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS.SSSSSSSSSSSSSSSSSSSSSSSSSSSSS 1708 / 1721 ( 99%)
SSSSSSSSSSSSS                                                 1721 / 1721 (100%)
Legend: ?-unknown, I-invalid file syntax, file ignored, S-Skipped, .-no changes, F-fixed, E-error
   1) /home/thomas/Documents/github/PrestaShop/src/PrestaShopBundle/Model/AdminModelAdapter.php (no_blank_lines_after_phpdoc, no_extra_blank_lines, single_blank_line_before_namespace, braces)

Checked all files in 2.134 seconds, 32.000 MB memory used

Some files need to be fixed!

Please run the following script to fix them. Afterwards, add the changed files and commit again:


    php ./vendor/bin/php-cs-fixer fix

    

It works as expected. 🙂
I could merge it, but before doing so, I'd like to report 2 things:

  • I did not expect so much verbosity from the script, shall we keep the --show-progress=dots parameter?
  • I would replace the last message with:
Please run the following script to fix them:
    php ./vendor/bin/php-cs-fixer fix
Afterwards, add the changed files and commit again.
@eternoendless

This comment has been minimized.

Copy link
Member Author

eternoendless commented Jan 8, 2019

I did not expect so much verbosity from the script

I added it because it can take a long time when the cache is invalidated (eg when switching branches), but I agree it may be too verbose.

I would replace the last message with...

That's how I did it in my first version, but then I decided to change it because I think most people will stop reading after the php ./vendor/bin/php-cs-fixer fix line.

This is how it looked in my console:

Checked all files in 98.407 seconds, 38.000 MB memory used

Some files need fixing!

Please run:


    php ./vendor/bin/php-cs-fixer fix

    
Afterwards, add the fixed files and commit again.
17:28:43 [prestamac-026] pBorowicz [improve-pre-commit] ~/repos/prestashop $
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 8, 2019

That was because there are whitespaces between the command and the other part of text. But anyway, let's start with this version. :)

@Quetzacoalt91 Quetzacoalt91 merged commit bd1421e into PrestaShop:develop Jan 8, 2019

1 check passed

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

@eternoendless eternoendless deleted the eternoendless:improve-pre-commit branch Jan 10, 2019

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