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

[CS] Update config for php-cs-fixer v2.0 and remove StyleCI config/badge #992

Closed
wants to merge 7 commits into from

Conversation

shakaran
Copy link

Q A
Branch? 1.0
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #927 (comment)
License MIT
Doc PR

This is for the comment in PR #927 to use again PHP CS Fixer 2.0 instead 1.0

.php_cs Outdated
//'ordered_class_elements' => true,
'ordered_imports' => true,
'php_unit_construct' => true,
'php_unit_dedicate_assert' => true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation error.

.php_cs Outdated
->setRules([
//'@PHP56Migration' => true,
'@Symfony' => true,
//'@Symfony:risky' => true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this enable? Do we possibly want to do so? We generally strictly follow Symfony's coding standings, short of long array syntax and a few other small differences.

Copy link
Author

Choose a reason for hiding this comment

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

It is commented, just in case that you or someone want enable to experiment. The risky enable more agressive fixes, that not in all cases could be perfect. According to doc "Risky rule is a rule, which could change code behaviour. By default no risky rules are run."

.php_cs Outdated
$config = PhpCsFixer\Config::create()
->setRiskyAllowed(true)
->setRules([
//'@PHP56Migration' => true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get rid of all commented entries?

Copy link
Author

Choose a reason for hiding this comment

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

ok, I leave it just in case that you want enable someone of them, that actually could be good for the project. I will remove all commented

@robfrawley robfrawley changed the title Update php cs fixer v2 0 [CS] Update config for php-cs-fixer v2.0 and remove StyleCI config/badge Sep 12, 2017
@robfrawley robfrawley modified the milestones: 1.10.0, 1.9.2 Sep 12, 2017
.php_cs Outdated
'@Symfony' => true,
'array_syntax' => ['syntax' => 'long'],
'combine_consecutive_unsets' => true,
'linebreak_after_opening_tag' => true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line (18), as well as line 22, 23, and 24 are indented using \t (tab) characters. Please ensure the whole document is written out using spaces.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just corrected it through the web interface

@robfrawley
Copy link
Collaborator

@shakaran You need to change the version constraint for friendsofphp/php-cs-fixer to from ~1.0 to ~2.0 on line 38 of our composer.json file.

.php_cs Outdated
@@ -0,0 +1,34 @@
<?php
$header = <<<'EOF'

Copy link
Collaborator

Choose a reason for hiding this comment

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

This blank line needs to be removed, and the spaces before the other header lines need to be removed, as well. It should be:

$header = <<<'EOF'
This file is part of the `liip/LiipImagineBundle` project.

(c) https://github.com/liip/LiipImagineBundle/graphs/contributors

For the full copyright and license information, please view the LICENSE.md
file that was distributed with this source code.
EOF;

@@ -0,0 +1,34 @@
<?php
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a newline after the opening <?php and add our copyright, making it:

<?php

/*
 * This file is part of the `liip/LiipImagineBundle` project.
 *
 * (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
 *
 * For the full copyright and license information, please view the LICENSE.md
 * file that was distributed with this source code.
 */

$header = <<<'EOF'

.php_cs Outdated
@@ -12,42 +12,17 @@ EOF;
$config = PhpCsFixer\Config::create()
->setRiskyAllowed(true)
->setRules([
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to obey the cs rules defined here, in this file, itself. :-) Change to long array syntax here, and elsewhere in the file where short array syntax is used.

@robfrawley
Copy link
Collaborator

robfrawley commented Sep 20, 2017

@shakaran @antoligy @cedricziel I think we should take this opportunity to update our current rule set and add some additional ones that have merit. The ruleset I'd like to run past everyone is the following:

array(
    '@Symfony' => true,
    '@Symfony:risky' => true,
    'array_syntax' => array('syntax' => 'long'),
    'combine_consecutive_unsets' => true,
    'header_comment' => array('header' => $header),
    'heredoc_to_nowdoc' => true,
    'linebreak_after_opening_tag' => true,
    'list_syntax' => array('syntax' => 'long'),
    'no_short_echo_tag' => true,
    'no_unreachable_default_argument_value' => true,
    'no_useless_else' => true,
    'no_useless_return' => true,
    'ordered_class_elements' => true,
    'ordered_imports' => true,
    'php_unit_construct' => true,
    'php_unit_dedicate_assert' => true,
    'phpdoc_add_missing_param_annotation' => true,
    'phpdoc_order' => true,
    'psr4' => true,
    'strict_comparison' => true,
    'strict_param' => true,
)

Give robfrawley@e172b74 a look to help in visualizing all the changes that would be introduced by the ruleset changes noted above (the commit also includes a few manual changes I made to some bad comparison operations that were otherwise broken by these rules, but such changes only need to be made once to resolve the issue and are for the better, anyway).

Note: the yoda-style changes aren't actually part of any of the ruleset changes here; instead, the @Symfony group recently enabled them and those will be applied regardless of the changes I'm proposing.

@shakaran
Copy link
Author

shakaran commented Oct 6, 2017

@robfrawley so I need to add something more to this PR to be merged? Or I have to update with the ruleset that you indicated?

@peter-gribanov
Copy link
Contributor

peter-gribanov commented Nov 27, 2017

I am just curious.
Why did you decide to give up Style-CI?
php-cs-fixer helps to fix CS in development, but does not check CS in PR.
Or i don't know something?

@lsmith77
Copy link
Contributor

another candidate for 2.0

@sebastianblum
Copy link
Contributor

I created the Pull request #1040 in the 2.0 branch :)

@lsmith77
Copy link
Contributor

thx

@maximgubar
Copy link
Contributor

@shakaran could you please update your MR taking into account @robfrawley's comments ?

@dbu
Copy link
Member

dbu commented Aug 9, 2019

lets not touch cs in legacy code anymore.

@dbu dbu closed this Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants