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

Upgrade to Slevomat CS 5.0 #109

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Feb 7, 2019

This only moves us to Slevomat CS 5.0 - no sniffs are added, changed or removed.

New sniffs and changes coming in upcoming PRs.

@Majkl578 Majkl578 added this to the 6.0.0 milestone Feb 7, 2019
@Majkl578 Majkl578 requested a review from a team as a code owner February 7, 2019 15:06
Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Thanks for organising the order of sniffs, my OCD really appreciates it 👍

tests/expected_report.txt Show resolved Hide resolved
tests/fixed/UselessConditions.php Show resolved Hide resolved
}

public function uselessIfConditionWithBoolMethod() : bool
{
return ! $this->isTrue();
if ($this->isTrue()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See slevomat/coding-standard#604 - CS doesn't know the type of $this->isTrue() and in general it may produce broken code (the method would return non-bool). This is now still reported, but not auto-fixed.

@@ -70,7 +78,7 @@ public function uselessIfConditionWithComplexCondition() : bool
public function uselessIfConditionWithTernary() : bool
{
if ($this->isTrue()) {
return $this->isTrulyTrue();
return $this->isTrulyTrue() ? true : false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See slevomat/coding-standard#604 - CS doesn't know the type of $this->isTrulyTrue() and in general it may produce broken code (the method would return non-bool). This is now still reported, but not auto-fixed.

@@ -103,12 +111,12 @@ public function uselessTernary() : bool

public function uselessTernaryWithParameter(bool $condition) : bool
{
return $condition;
return $condition ? true : false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See slevomat/coding-standard#604 - CS doesn't know the type of $condition and in general it may produce broken code (the method would return non-bool). This is now still reported, but not auto-fixed.

}

public function uselessTernaryWithMethod() : bool
{
return $this->isFalse();
return $this->isFalse() ? true : false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See slevomat/coding-standard#604 - CS doesn't know the type of $this->isFalse() and in general it may produce broken code (the method would return non-bool). This is now still reported, but not auto-fixed.

@ostrolucky
Copy link
Member

If these are no longer fixed, shouldn't it be removed from fixed/ folder?

@Majkl578
Copy link
Contributor Author

Majkl578 commented Feb 7, 2019

No, they are still used for the tests of errors produced. They just can't be fixed anymore.

@ostrolucky
Copy link
Member

isn't that what is input/ folder used for?

@Majkl578
Copy link
Contributor Author

Majkl578 commented Feb 7, 2019

Yes, but in Apply fixes stage, phpcbf is ran over input/ and diffed against fixed/.

@carusogabriel carusogabriel mentioned this pull request Feb 7, 2019
@lcobucci lcobucci self-assigned this Feb 7, 2019
@lcobucci lcobucci merged commit 7e110bb into doctrine:master Feb 7, 2019
@Majkl578 Majkl578 deleted the slevomat-cs-5.0 branch February 8, 2019 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants