Add the native_function_invocation fixer in the Symfony:risky ruleset #3873

stof · 2018-07-05T11:21:50Z

Symfony wants this fixer applied for compiler-optimized functions (we were the ones requesting the feature).

stof · 2018-07-05T11:23:07Z

Note that the Symfony codebase itself is not yet updated for this CS rule as we were waiting for the automatic fixer before making this part of our official coding standards (doing it manually was too painful so we were doing it only in the hot paths).
I'm currently preparing the PR running it.

keradus · 2018-07-05T11:36:06Z

hi @stof, great to see you here again ;)
yeah, discussion about this rule, it's expectation and configuration was longs indeed, finally we manage to cover (most of) users requirements, not only one from Sf ;)
native_constant_invocation was already added ;)

be aware that tests are failing, do you want to handle it yourself ?

SpacePossum · 2018-07-05T11:39:04Z

I'm currently preparing the PR running it.

looking forward to that diff ^_^

stof · 2018-07-05T11:41:09Z

I was fixing the tests, but I see that @SpacePossum already did it.

stof · 2018-07-05T12:46:13Z

@SpacePossum the Symfony PR is referenced just before my comment, in case you want to look at it.

keradus · 2018-07-05T12:47:49Z

.php_cs.dist

@@ -43,6 +43,7 @@ $config = PhpCsFixer\Config::create()
        'method_argument_space' => ['on_multiline' => 'ensure_fully_multiline'],
        'method_chaining_indentation' => true,
        'multiline_comment_opening_closing' => true,
+        'native_function_invocation' => false,


to be dropped when number of open PRs shrinks down a bit

we do follow Sf ruleset and add some more strict rules on top of it, but we do not exclude things from Sf ruleset.

could be another good argument to let #3798 pass

#3798 is indeed the one doing this removal (when it was adding it manually)

indeed, silly me, forgot about it...
I added milestone there to not lose track of it

keradus · 2018-07-06T09:20:14Z

looks good, yet we have always pushed change of rulesets to MINOR release, changing target to 2.13@dev

SpacePossum · 2018-07-06T09:26:32Z

👍
I do feel we are going to get a lot of questions about this when it gets shipped as it will be enabled for a lot of users, like it was done with Yoda, but we'll see :)

keradus · 2018-07-06T09:35:28Z

it;s really fair config, it does not enforce everything, just what is optimized by compiler.
all good imo

stof · 2018-07-10T18:34:31Z

@SpacePossum the Symfony rule is also configured to favor readability: we only apply it to the compiler-optimized functions, and only in namespaced code (for non-namespaced code, calls are always fully-qualified, and so always optimized).

Btw, that's also the reason for my change in #3876

Symfony wants this fixer applied for compiler-optimized functions.

SpacePossum · 2018-07-11T07:22:15Z

Thank you @stof.

…isky ruleset (stof) This PR was merged into the 2.13-dev branch. Discussion ---------- Add the native_function_invocation fixer in the Symfony:risky ruleset Symfony wants this fixer applied for compiler-optimized functions (we were the ones requesting the feature). Commits ------- 8556bee Add the native_function_invocation fixer in the Symfony:risky ruleset

@nicolas-grekas

This PR was squashed before being merged into the 2.8 branch (closes #27852). Discussion ---------- Fix coding standards | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This PR is mostly about running the PHP-CS-Fixer (v2.12.1) in the whole codebase. - I updated the exclude rule to avoid some false positives for the `error_suppression` fixer (we have more files triggering unsilenced deprecations on purpose than when building the initial whitelist, mostly). - I ran the fixer with this updated config. Most changes were related to fully-qualifying some constants, with the new fixer implemented in PHP-CS-Fixer/PHP-CS-Fixer#3127, for which @nicolas-grekas and I suggested a config to include in the Symfony ruleset. Based on the output, I suggested a feature request in PHP-CS-Fixer/PHP-CS-Fixer#3872 as we might want to avoid the `\` in non-namespaced files to improve readability. We might want to remove the second commit of this PR if we decide to wait for the feature to be implemented (update: implementation is contributed in PHP-CS-Fixer/PHP-CS-Fixer#3876) - I added the `native_function_invocation` fixer explicitly, to automatically fully-qualify calls to compiler-optimized functions. This feature was implemented in PHP-CS-Fixer based on our feature request (as currently, we do such thing only manually in some hot path, because it could not be automated). I opened PHP-CS-Fixer/PHP-CS-Fixer#3873 to include it in the ruleset automatically. TODOs: - [x] agree on the updated rules - [x] update fabbot to use the new version of PHP-CS-Fixer - [ ] make separate PRs for newer branches with their own updates (exclude rules, and CS fixes), once this PR gets merged. Commits ------- 538c69d Fix Clidumper tests 04654cf Enable the fixer enforcing fully-qualified calls for compiler-optimized functions f00b327 Apply fixers 720ed4d Disable the native_constant_invocation fixer until it can be scoped 8892b98 Update the list of excluded files for the CS fixer

@nicolas-grekas

This PR was squashed before being merged into the 2.8 branch (closes #27852). Discussion ---------- Fix coding standards | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This PR is mostly about running the PHP-CS-Fixer (v2.12.1) in the whole codebase. - I updated the exclude rule to avoid some false positives for the `error_suppression` fixer (we have more files triggering unsilenced deprecations on purpose than when building the initial whitelist, mostly). - I ran the fixer with this updated config. Most changes were related to fully-qualifying some constants, with the new fixer implemented in PHP-CS-Fixer/PHP-CS-Fixer#3127, for which @nicolas-grekas and I suggested a config to include in the Symfony ruleset. Based on the output, I suggested a feature request in PHP-CS-Fixer/PHP-CS-Fixer#3872 as we might want to avoid the `\` in non-namespaced files to improve readability. We might want to remove the second commit of this PR if we decide to wait for the feature to be implemented (update: implementation is contributed in PHP-CS-Fixer/PHP-CS-Fixer#3876) - I added the `native_function_invocation` fixer explicitly, to automatically fully-qualify calls to compiler-optimized functions. This feature was implemented in PHP-CS-Fixer based on our feature request (as currently, we do such thing only manually in some hot path, because it could not be automated). I opened PHP-CS-Fixer/PHP-CS-Fixer#3873 to include it in the ruleset automatically. TODOs: - [x] agree on the updated rules - [x] update fabbot to use the new version of PHP-CS-Fixer - [ ] make separate PRs for newer branches with their own updates (exclude rules, and CS fixes), once this PR gets merged. Commits ------- 538c69dc26 Fix Clidumper tests 39d2079 Enable the fixer enforcing fully-qualified calls for compiler-optimized functions f00b3279ea Apply fixers 720ed4d379 Disable the native_constant_invocation fixer until it can be scoped 8892b98627 Update the list of excluded files for the CS fixer

stof mentioned this pull request Jul 5, 2018

Fix coding standards symfony/symfony#27852

Merged

3 tasks

SpacePossum added the kind/enhancement label Jul 5, 2018

keradus reviewed Jul 5, 2018

View reviewed changes

keradus added this to the 2.13.0 milestone Jul 6, 2018

keradus changed the base branch from 2.12 to master July 6, 2018 09:20

keradus approved these changes Jul 6, 2018

View reviewed changes

keradus added the RTM Ready To Merge label Jul 10, 2018

SpacePossum force-pushed the update_symfony_ruleset branch 2 times, most recently from 2eec764 to d333905 Compare July 11, 2018 07:19

Add the native_function_invocation fixer in the Symfony:risky ruleset

8556bee

Symfony wants this fixer applied for compiler-optimized functions.

SpacePossum force-pushed the update_symfony_ruleset branch from d333905 to 8556bee Compare July 11, 2018 07:20

SpacePossum merged commit 8556bee into PHP-CS-Fixer:master Jul 11, 2018

SpacePossum removed the RTM Ready To Merge label Jul 11, 2018

stof deleted the update_symfony_ruleset branch July 24, 2018 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the native_function_invocation fixer in the Symfony:risky ruleset #3873

Add the native_function_invocation fixer in the Symfony:risky ruleset #3873

stof commented Jul 5, 2018

stof commented Jul 5, 2018

keradus commented Jul 5, 2018

SpacePossum commented Jul 5, 2018

stof commented Jul 5, 2018

stof commented Jul 5, 2018

keradus Jul 5, 2018 •

edited

SpacePossum Jul 5, 2018 •

edited

stof Jul 5, 2018

keradus Jul 6, 2018

keradus commented Jul 6, 2018

SpacePossum commented Jul 6, 2018

keradus commented Jul 6, 2018

stof commented Jul 10, 2018

SpacePossum commented Jul 11, 2018

Add the native_function_invocation fixer in the Symfony:risky ruleset #3873

Add the native_function_invocation fixer in the Symfony:risky ruleset #3873

Conversation

stof commented Jul 5, 2018

stof commented Jul 5, 2018

keradus commented Jul 5, 2018

SpacePossum commented Jul 5, 2018

stof commented Jul 5, 2018

stof commented Jul 5, 2018

keradus Jul 5, 2018 • edited

Choose a reason for hiding this comment

SpacePossum Jul 5, 2018 • edited

Choose a reason for hiding this comment

stof Jul 5, 2018

Choose a reason for hiding this comment

keradus Jul 6, 2018

Choose a reason for hiding this comment

keradus commented Jul 6, 2018

SpacePossum commented Jul 6, 2018

keradus commented Jul 6, 2018

stof commented Jul 10, 2018

SpacePossum commented Jul 11, 2018

keradus Jul 5, 2018 •

edited

SpacePossum Jul 5, 2018 •

edited