fix core folder with php-cs-fixer for psr-2 #7956
Conversation
|
Is there a way on Github to see the full diff? Does anyone know that? |
|
https://patch-diff.githubusercontent.com/raw/piwik/piwik/pull/7956.diff I think will work. |
|
Thx @diosmosis . It looks good to me in general. Especially was interested in making sure nothing changed re It does sometimes remove quite a few lines that I would maybe personally rather prefer to keep. Eg if ($pluginName) {
-
if ($this->pluginContainsJScriptAssets($pluginName)) {
-
if (Manager::getInstance()->isPluginBundledWithCore($pluginName)) {
-
$assetsToRemove[] = $this->getMergedCoreJSAsset();
-
} else {
-
$assetsToRemove[] = $this->getMergedNonCoreJSAsset();
}
}
-
} else {
-
$assetsToRemove[] = $this->getMergedCoreJSAsset();
$assetsToRemove[] = $this->getMergedNonCoreJSAsset();
}Sometimes such lines are really good to structure code and make it better readable. Is it maybe possible to ignore new lines? Anyone else has any thoughts on that? |
|
@tsteur It might be configurable. But I wouldn't recommend it. It would still be valid PSR-2 code with that lines, but PSR recommends adding lines to separate the code in blocks, which is not happening in the example you provided. Much better approach is to use early returns and completely remove all else statements. The code above could be translated to this code below, which does exactly the same thing. if (! $pluginName) {
$assetsToRemove[] = $this->getMergedNonCoreJSAsset();
$assetsToRemove[] = $this->getMergedCoreJSAsset();
return;
}
if (! $this->pluginContainsJScriptAssets($pluginName)) {
return;
}
if (Manager::getInstance()->isPluginBundledWithCore($pluginName)) {
$assetsToRemove[] = $this->getMergedCoreJSAsset();
return;
}
$assetsToRemove[] = $this->getMergedNonCoreJSAsset();Anyway, that was generated using an automated tool. And this kind of code style improvement should be done in another PR. |
|
The code looks nicer that way! It was only an example though. If we remove it there, we also remove it where it makes sense and where it is good to have it. Adding it afterwards won't happen. I'd say we should rather leave the new lines there and remove the unneeded ones on the fly when working on a file anyway. Does anyone else have an opinion on that? |
|
By removing that empty lines you will be removing them where they shouldn't be. Also probably there are many cases when the removal won't "affect readability". If you simple ignore that, you may never remember to do that. I would prefer to let the tool fix them now, since it's the default setting for PSR-2 and work on the code when you guys find a readability problem. Also, the code will still be readable even without those lines. |
fabiocarneiro
force-pushed
the
psr-2-code-style-fixes
branch
3 times, most recently
from
6331969
to
f13e1ae
Compare
|
I rebased it again. Please someone merge this. Since there are many files it will get conflicts very fast. |
| @@ -89,7 +88,8 @@ protected function manipulateDataTable($dataTable) | |||
|
|
|||
| $metricNames = array(); | |||
| foreach ($metricsToCalculate as $metricId) { | |||
| $metricNames[$metricId] = Metrics::getReadableColumnName($metricId);; | |||
| $metricNames[$metricId] = Metrics::getReadableColumnName($metricId); | |||
| ; | |||
|
Note: the build is failing for this PR, maybe the rebase went wrong? https://travis-ci.org/piwik/piwik/jobs/64199263 I'm not sure what happened there but FYI this build failure is related to the code changed in #7992 Once build is green It's +1 for me to merge |
fabiocarneiro
force-pushed
the
psr-2-code-style-fixes
branch
from
f13e1ae
to
a2a63d3
Compare
|
@mattab I don't know why the build is failing since these are only code style fixes. To make sure I reverted the commit, and ran the command again, but this build error seems to be random. The things you complained above I had to fix manually. Php-cs-fixer seems not no have a fixer for that. I guess this is ok to merge. But it would be nice to investigate why this "Piwik\Tests\System\DuplicateActionsTest" is failing. I've already seen it in many builds. |
mattab
added
the
Task
fix core folder with php-cs-fixer for psr-2
|
Thanks @fabiocarneiro for the PR 👍 |
|
See also our follow up issue |
This fixes all the psr-2 problems at once.
This was generated using php-cs-fixer with psr-2 level plus unused_use filter.
Here is a list of which problem was found in each file.