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.

Sign up for GitHub

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

Jump to bottom

AssertEqualsIsDiscouraged #6

Open
wants to merge 1 commit into
base: 1.1.x
Choose a base branch
from
Open

AssertEqualsIsDiscouraged #6

wants to merge 1 commit into from

Conversation

mhujer
Copy link
Contributor

@mhujer mhujer commented Dec 27, 2017

This rule is even more opinionated than the previous ones.

If you don't think it should be included in PHPStan, I can change it and keep only the first part (which suggests assertSame in for the same types)

ondrejmirtes
ondrejmirtes requested changes Dec 28, 2017
View reviewed changes
src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php
if (
($leftType instanceof BooleanType && $rightType instanceof BooleanType)
|| ($leftType instanceof IntegerType && $rightType instanceof IntegerType)
|| ($leftType instanceof StringType && $rightType instanceof StringType)
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to simplify this condition with Type::isSupersetOf() and a UnionType consisting of bool, int and string. cc @JanTvrdik might help you with that.

Copy link
Contributor Author

@mhujer mhujer Dec 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if the union would mean the same thing here. Both (left and right) need to be of the same type (not in the union of bool|int|string)

Copy link
Contributor

@Jean85 Jean85 Aug 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe union + \get_class($leftType) === \get_class($rightType)?

src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php Outdated
if (
($leftType instanceof FloatType && $rightType instanceof FloatType)
&&
count($node->args) < 4 // is not using delta for comparing floats
Copy link
Member

Choose a reason for hiding this comment

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

This newline isn't necessary.

src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php Outdated
}

$fileContents = explode("\n", file_get_contents($scope->getFile()));
$previousLine = $fileContents[$node->getLine() - 2];
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this. You should look into $node properties instead, there should be a comment available.

@ondrejmirtes
Copy link
Member

I submitted a review 😊 Also please rebase your branch on master and fix PHP 7.1-related CS errors.

@mhujer mhujer force-pushed the mh-strict-rules-assertEquals branch 2 times, most recently from f9b30da to bb05c60 Compare December 28, 2017 16:17
@mhujer mhujer mentioned this pull request Dec 28, 2017
Open
4 tasks
@mhujer
Copy link
Contributor Author

mhujer commented Dec 28, 2017

R4R @ondrejmirtes (I kept the fixes in the separate commits for easier review)

@mhujer mhujer force-pushed the mh-strict-rules-assertEquals branch from bb05c60 to ac956fe Compare January 21, 2018 16:31
@mhujer mhujer force-pushed the mh-strict-rules-assertEquals branch from ac956fe to 433dacf Compare February 2, 2018 07:42
@mhujer
Copy link
Contributor Author

mhujer commented Feb 2, 2018

Any feedback on this one?

@mhujer mhujer force-pushed the mh-strict-rules-assertEquals branch from 433dacf to 122e716 Compare February 11, 2018 14:47
@mhujer mhujer force-pushed the mh-strict-rules-assertEquals branch 3 times, most recently from c315145 to 0e9a3a4 Compare March 5, 2018 14:01
@Majkl578
Copy link
Contributor

TBH I don't think this belongs here directly, it should be in phpstan-strict-rules (or i.e. phpstan-phpunit-strict-rules), or at least in separate ruleset (i.e. strict-rules.neon).

@mhujer mhujer force-pushed the mh-strict-rules-assertEquals branch from 0e9a3a4 to 61291dc Compare November 18, 2019 14:00
@ikvasnica
Copy link

I just made a very similar rule to my package, check it out and feel free to comment.

It forbids assertEquals with any scalar type, no matter whether the two arguments are of the same type or not.

ikvasnica/phpstan-clean-test#12

@mhujer mhujer force-pushed the mh-strict-rules-assertEquals branch from af60919 to e8572b7 Compare December 30, 2020 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jean85 Jean85 Jean85 left review comments

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mhujer @ondrejmirtes @Majkl578 @ikvasnica @Jean85