[CS][2.7] yoda_style, no_unneeded_curly_braces, no_unneeded_final_method, semicolon_after_instruction #24123
Conversation
|
yoda_style is nice |
--- Original
+++ New
@@ @@
<?php
final class Foo {
- final public function foo() {}
- final protected function bar() {}
- final private function baz() {}
+ public function foo() {}
+ protected function bar() {}
+ private function baz() {}
}--- Original
+++ New
@@ @@
-<?php {
+<?php
echo 1;
-}
switch ($b) {
- case 1: {
+ case 1:
break;
- }
+
} |
|
just to complete the picture: --- Original
+++ New
@@ @@
-<?php echo 1 ?>
+<?php echo 1; ?> |
|
I believe the Example from the source protected function getVoterFor2Roles($token, $vote1, $vote2)
{
$voter = $this->getMockBuilder('Symfony\Component\Security\Core\Authorization\Voter\VoterInterface')->getMock();
$voter->expects($this->any())
->method('vote')
->will($this->returnValueMap(array(
array($token, null, array('ROLE_FOO'), $vote1),
array($token, null, array('ROLE_BAR'), $vote2),
- )))
- ;
+ )));
return $voter;
} |
|
The case given by @iltar looks wrong to me. The semi-colon was here, even though there was a newline before it. It is not a missing semi-colon. |
|
|
|
My case was a question if that would be the case as well (sorry if I put it a bit weird), I manually diffed it to see if my assumption was correct about the rule. |
|
all good :) |
|
I'm not even sure if this is something that rule (with the I don't think the check |
|
Oh btw, we should disable this (I mean, php-cs-fixer should not do that): final class Foo {
- protected function bar() {}
+ private function bar() {}
} |
|
|
|
I mean this should not be done by cs-fixer (it is right now, I already saw a false-positive by fabbot on a PR) |
I'm definitely 👍 on that one, that will save us hundred of comments reminding we follow yoda style |
|
btw, if you know how to fix this false positive, that'd be great: |
what's the purpose of protected method inside of final class that does not inherit ? |
|
The use case is easy: just try changing that with the existing code in CacheItem and you'll see by looking at tests. Hint: it relates to casting to arrays, which does not produce the same result in both cases. |
|
I analysed the scenario. IMHO, accessing protected $item = (array) $item;
$key = $item["\0*\0key"];is abusing the language to use The rule behind the fixer is good in general, you just hint edge cases, where it doesn't work for you due to using that workaround to get encapsulated property. I was looking how to fix $this->createCacheItem = \Closure::bind(
function ($key, $value, $isHit) use ($defaultLifetime) {
$item = new CacheItem();
$item->key = $key;
$item->value = $value;
$item->isHit = $isHit;
$item->defaultLifetime = $defaultLifetime;
return $item;
},
null,
CacheItem::class
);what's the thing here, why it has to abuse OOP that much ? |
|
If you want to have that rule, it belongs to a "code quality" tool - not to a CS fixer. |
|
Fixer went far beyond touching whitespaces only ages ago ;) Yet, you did not answer my question :( |
|
👍 for all new proposed rules |
|
@keradus I did not answer your question because it's off topic for this PR. Yet, you say something is abusing OOP. To me, nothing does. Accessing private state internally is not abusing OOP. That's what it is here. Of course, "private" here doesn't mean "private by PHP rules". But still it's private - internal detail if you prefer. PHP is just a tool. If we were in C++, we would use friendship. Here, we do with what we have to achieve the same. SOLID everywhere. |
|
you raised a concern and pointed the class, so i did investigated it ;) |
|
I agree with @keradus on this one. The solution is more of a hack and an exception (due to limited features). Protected functions in a final class make no sense, if this is done on purpose, it should explicitly be excluded from the check and also documented why (bus/truck/plane/etc factor). |
|
What matters to me is fixing this false positive. An exclusion would do it so I'm fine with it. |
big 👍 |
|
@SpacePossum can you open another PR to just enable the new rules (without applying them on the code base)? Than would enforce them for the new PRs, that'd be a good first step. |
|
@nicolas-grekas that wouldn't work, but no worries. Since we've established that these rules should go the SF ruleset I've opened; |
This PR was merged into the 3.3 branch. Discussion ---------- [CS] Remove `protected_to_private` rule | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | n/a | License | MIT Following: #24123 (comment) I picked 3.1 as target as the rule discussed; | branch | hits | --------- | --- | 2.7 | 0 | 2.8 | 0 | 3.0 | 0 | 3.1 | 1 | master | 1 hit (same file as on 3.1) Commits ------- bfae454 Remove `protected_to_private` rule.
|
|
e8f4a36 to
6e2237a
Compare
6e2237a to
f65fd69
Compare
|
Thank you @SpacePossum. |
…ded_final_method, semicolon_after_instruction (SpacePossum) This PR was squashed before being merged into the 2.7 branch (closes #24123). Discussion ---------- [CS][2.7] yoda_style, no_unneeded_curly_braces, no_unneeded_final_method, semicolon_after_instruction | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Hi all and congratulations on the 1 billion of downloads! awesome :D This is a courtesy call (or PR) from the PHP-CS-Fixer community. As always I don't expect this PR to be merged nor do I've any strong feelings about why not. The goal of this PR is to determine if the following new rules should be added to the Symfony ruleset by default. If so `fabbot.io` will (at some point) start reporting violations of these rules. Please be aware that if the rules are accepted but this PR is not, any new PR's might contain a part of the fixes as proposed in this PR as fabbot.io will suggest to the authors of the new PR's to update the CS, consequently this might create noise. Here are the rules: ``` 'no_unneeded_curly_braces' => true, 'no_unneeded_final_method' => true, 'semicolon_after_instruction' => true, 'yoda_style' => true, ``` The first three are done in the first commit. The `yoda_style` in the last. I know the `yoda`-style fixing has a long history about it and some people have strong opinions on it. Please consider two things before commenting about Yoda - this PR is not meant to determine the Symfony code style, I'm sure Symfony has other channels for these kind of discussions - if you don't like Yoda style, please consider heading over to PHP-CS-Fixer/PHP-CS-Fixer#2446 and review my PR. If it gets merged you'll have a free tool to change all Yoda expressions in your code to non-Yoda (you can do it around as well as you can see in both PR's). Thank you for your time! Commits ------- 3e90138 [CS][2.7] yoda_style, no_unneeded_curly_braces, no_unneeded_final_method, semicolon_after_instruction
|
now applied up to master |
This PR was merged into the 2.7 branch. Discussion ---------- CS: recover no_break_comment | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | n/a, see CIs | Fixed tickets | n/a | License | MIT | Doc PR | n/a rule was excluded in #24123, yet without any information about it it was not excluded in original form of given PR, it's not part of any commitlog nor any comment, title claims to touch only other rules. this rule shall not be excluded, it's part of PSR and is applied for most places. I added missing ones. Commits ------- 4e9d160 CS: recover no_break_comment
Hi all and congratulations on the 1 billion of downloads! awesome :D
This is a courtesy call (or PR) from the PHP-CS-Fixer community.
As always I don't expect this PR to be merged nor do I've any strong feelings about why not.
The goal of this PR is to determine if the following new rules should be added to the Symfony ruleset by default. If so
fabbot.iowill (at some point) start reporting violations of these rules.Please be aware that if the rules are accepted but this PR is not, any new PR's might contain a part of the fixes as proposed in this PR as fabbot.io will suggest to the authors of the new PR's to update the CS, consequently this might create noise.
Here are the rules:
The first three are done in the first commit.
The
yoda_stylein the last.I know the
yoda-style fixing has a long history about it and some people have strong opinions on it. Please consider two things before commenting about YodaThank you for your time!