Navigation Menu

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

Update PHP CS Fixer #11718

Closed
wants to merge 1 commit into from
Closed

Update PHP CS Fixer #11718

wants to merge 1 commit into from

Conversation

MathiasReker
Copy link
Contributor

@MathiasReker MathiasReker commented Dec 11, 2018

Questions Answers
Branch? develop
Description? /
Type? improvement
Category? CO
BC breaks? Does it break backward compatibility? no
Deprecations? Does it deprecate an existing feature? no
Fixed ticket? /
How to test? /
Rule PR
align_multiline_comment' => ['comment_type' => 'phpdocs_only'] #11694
'array_indentation' => true #11695
'combine_consecutive_issets' => true #11696
'combine_consecutive_unsets' => true #11697
'hash_to_slash_comment' => true #11699
'logical_operators' => true #11701
'no_short_echo_tag' => true #11703
'no_alias_functions' => true #11707
'modernize_types_casting' => true #11708
'no_mixed_echo_print' => true #11709
'no_closing_tag' => true #11738
'single_blank_line_at_eof' => true #11738

This change is Reviewable

@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Dec 11, 2018
@MathiasReker
Copy link
Contributor Author

WDYT?

This was referenced Dec 11, 2018
@PierreRambaud
Copy link
Contributor

@PrestaShop/prestashop-core-developers Need everyone feedback :)

@Quetzacoalt91
Copy link
Member

Quetzacoalt91 commented Dec 12, 2018

I won't say no to additional checks, especially if they can be fixed by CS fixer. :)

@jolelievre
Copy link
Contributor

This new setting seems good to me, and thank you for all the other PRs where you cleaned all the code!
I think we should wait to merge all the other PRs before this one.

@Quetzacoalt91
Copy link
Member

Of course, this one should not pass the linter jobs until the other PRs are merged. :)

@AJenbo
Copy link

AJenbo commented Dec 14, 2018

Have you guys considered setting up StyleCI? It runs php-cs-fixer as a service.

@Quetzacoalt91
Copy link
Member

We are looking for tools, but haven't chosen yet. @eternoendless on its side heard about prettyci.com.

@AJenbo
Copy link

AJenbo commented Dec 14, 2018

Hm I think prettyci.com might actually be a better choice especially if they respect the .php_cs rather then require a separate format.

@MathiasReker
Copy link
Contributor Author

Please merge this PR.

@Quetzacoalt91
Copy link
Member

Hi @MathiasReker,

In order to go on this PR and approve your changes, I restarted the Travis build. Unfortunately it appears the linter jobs are currently failing.
This means more changes must be brought on this PR to make it pass.

Copy link
Member

@eternoendless eternoendless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some rules are missing:

  • no_short_echo_tag
  • no_alias_functions
  • modernize_types_casting
  • no_mixed_echo_print

.php_cs.dist Outdated Show resolved Hide resolved
@MathiasReker
Copy link
Contributor Author

If some rules are missing now, they are missing because they are already a symfony rule :-)

.php_cs.dist Outdated Show resolved Hide resolved
.php_cs.dist Outdated Show resolved Hide resolved
@PierreRambaud
Copy link
Contributor

Look like you include all @symfony:risky elements, as it says, it is risky :/ but use '@Symfony:risky' => true instead. And do not duplicate rules already in @Symfony, or @Symfony:risky

@MathiasReker
Copy link
Contributor Author

@PierreRambaud I am not sure I included all risky symfony rules, I picked them one by one. None of included risky rules seems to have any negative affect on on this project. Are you sure would use @Symfony:risky ? What if Cs fixer is updated with new risky rules?

@PierreRambaud
Copy link
Contributor

We have composer.lock which target a specific php-cs-fixer version. It is exactly the same as phpunit, Symfony and all dependencies. We control what we have, and so we should watch diff before updating dependencies.

@MathiasReker
Copy link
Contributor Author

@PierreRambaud I updated the configuration. Wdyt?

        'native_constant_invocation' => false,
        'native_function_invocation' => false,

@MathiasReker
Copy link
Contributor Author

WDYT?

I am not sure how to solve Travis CI lint.

@eternoendless
Copy link
Member

New commits add more checks than were initially proposed. Can we roll them back and go with the initial suggestion? We can discuss adding others in another PR.

@MathiasReker
Copy link
Contributor Author

@eternoendless I will do so. I am busy ATM. I will do it ASAP.

@MathiasReker
Copy link
Contributor Author

I messed something up, trying to squash my commits. New PR: #12922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants