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 advice when lint fails + Display all files in error in Travis lint job #11920

Merged
merged 2 commits into from Jan 15, 2019

Conversation

Projects
None yet
6 participants
@Quetzacoalt91
Copy link
Member

Quetzacoalt91 commented Dec 24, 2018

Questions Answers
Branch? develop
Description? When the lint jobs fails on Travis, we should tell the contributor how to fix the issue.
Type? improvement
Category? TE
BC breaks? Nope
Deprecations? Nope
Fixed ticket? /
How to test? To be tested locally by running bash tests-legacy/check_file_syntax.sh

This change is Reviewable

@@ -25,5 +25,8 @@ if [[ "$php" == "0" && "$yaml_src" == "0" && "$yaml_app" == "0" && "$yaml_themes
exit 0;
else
echo -e "\e[91mSYNTAX TESTS FAILED"
if [[ "$coding_styles" -ne "0" ]]; then
echo -e "\e[91mRun \"php ./vendor/bin/php-cs-fixer fix\" from your PrestaShop folder to fix the lint issues.";

This comment has been minimized.

@N-Wouda

N-Wouda Dec 24, 2018

Contributor

./vendor/bin/php-cs-fixer is a shell script, not PHP. The correct command should be ./vendor/bin/php-cs-fixer fix.

Note that it might be nice to inform the contributor they need to install it first via composer, e.g. composer install. Though it seems unlikely they'd not already have done so :).

This comment has been minimized.

@PierreRambaud

PierreRambaud Dec 26, 2018

Contributor

./vendor/bin/php-cs-fixer is a PHP script under Linux and begins with the shebang #!/usr/bin/env php

This comment has been minimized.

@N-Wouda

N-Wouda Dec 26, 2018

Contributor

@PierreRambaud I invite you to try this under Windows

This comment has been minimized.

@PierreRambaud

PierreRambaud Dec 26, 2018

Contributor

This script is written to work under Linux system, because of lines one and three and because of Travis which is running under Linux system too.

This comment has been minimized.

@N-Wouda

N-Wouda Dec 26, 2018

Contributor

But here's the thing: as it stands, with php explicitly mentioned, this does not work under Windows. It is superfluous--as you mentioned, the script already defines the environment--so it can safely be removed. I just checked this on Ubuntu to be safe, and indeed it still works fine after. Doing so, however, also makes it work under Windows. IIUC, there are no downsides here to dropping php and having the bot tell users to run ./vendor/bin/php-cs-fixer fix. That's nicely cross-platform. Do you agree?

This comment has been minimized.

@PierreRambaud

PierreRambaud Dec 26, 2018

Contributor

For that we should have a bat script (or don't know what powershell used) instead. Shell are created for Unix system only.

This comment has been minimized.

@N-Wouda

N-Wouda Dec 26, 2018

Contributor

I fear you might be confused, for which I apologise. Allow me to explain better.

I'm not talking about tests-legacy/check_file_syntax.sh. That's executed on Travis - fine by me. I'm talking about the recommendation given: that the contributor should run php ./vendor/bin/php-cs-fixer fix on his/her local dev machine. This specific command does not work under Windows, whereas ./vendor/bin/php-cs-fixer fix does. That still works under Linux, making it nicely cross-platform. As such, I think the recommendation should become ./vendor/bin/php-cs-fixer fix - i.e., only the line I commented on should be changed.

This comment has been minimized.

@PierreRambaud

PierreRambaud Dec 26, 2018

Contributor

Hahaha yes I was not thinking about that 🤣
In this case you're right, we can remove the php executable :)
My bad for the misunderstanding ^^

This comment has been minimized.

@N-Wouda

N-Wouda Dec 26, 2018

Contributor

Great, I'm glad we cleared that up!

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Dec 28, 2018

Author Member

This command does not work either because of the slashes. An actual command on the good old cmd is: .\vendor\bin\php-cs-fixer fix

@PierreRambaud
Copy link
Contributor

PierreRambaud left a comment

Change wording and you need to close the color line with \e[39m

@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Jan 14, 2019

php ./vendor/bin/php-cs-fixer fix --dry-run --stop-on-violation --show-progress=dot

I'd suggest to change it this way:

php ./vendor/bin/php-cs-fixer fix --dry-run --diff --diff-format=udiff

This will make it show a diff, so you can know what you were supposed to do (the format option is to show shorter diffs), and no longer stop at the first error, so you can account for all errors.

Note: The --show-progress option is useless unless the --verbose option is used too.

@Quetzacoalt91

This comment has been minimized.

Copy link
Member Author

Quetzacoalt91 commented Jan 14, 2019

Why not, this could be useful for #12111 :trollface:

@Quetzacoalt91 Quetzacoalt91 changed the title Add advice when lint fails Add advice when lint fails + Display all files in error in Travis lint job Jan 14, 2019

@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Jan 14, 2019

It turns out it failed because there was a php 5.6 incompatible expression in that class which wasn't being catched by the linter! I had to run php-cs-fixer locally with php 5.6 to realize that.

@matks matks merged commit c975042 into PrestaShop:develop Jan 15, 2019

1 check passed

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

This comment has been minimized.

Copy link
Contributor

matks commented Jan 15, 2019

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.6.0 milestone Jan 15, 2019

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