Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Finder] Make Comparator immutable#42137
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| $this->setTarget($target); | ||
| $this->setOperator($matches[1] ??'=='); | ||
| parent::__construct($target,$matches[1] ?:'=='); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
did you change ?? to ?: on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes.setOperator() accepted an empty string and I've made the constructor stricter in that regard.
c937eec to4f0089bCompare| } | ||
| $comparator =newComparator(); | ||
| $comparator =newComparator('some target'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We have 2 tests in one here. Those should be split (which will also allow usingexpectException to test the exception) instead of this old way of writing tests that does not follow the best practices.
And the test about an invalid operator should also be added for the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| /** | ||
| * @dataProvider getTestData | ||
| */ | ||
| publicfunctiontestTest($operator,$target,$match,$noMatch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The data provider for this test does not provide full coverage (it tests only one operator). Would it make sense to add other cases ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The tests for this class are indeed pretty sparse. I think the class is mainly tested by testing its subclasses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
4f0089b to0ff3971CompareThis PR was merged into the 4.4 branch.Discussion----------[Finder] Improve ComparatorTest| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets |#42137 (comment),#42137 (comment)| License | MIT| Doc PR | not necessaryCommits-------4c44dce [Finder] Improve ComparatorTest
Signed-off-by: Alexander M. Turek <me@derrabus.de>
fabpot commentedJul 18, 2021
Thank you@derrabus. |
0ff3971 tob83ead2Compare
The
Comparatorclass exposes setters for its properties$targetand$operator. I'd consider this design to be flawed for the following reasons:$targetproperty should be set before executing the main methodtest()on an instance of that class. However, we never make sure that this actually happens.DateComparatorandNumberComparatorset both properties via their constructors, after parsing them from a string. It's a bit odd to allow those properties to be overridden via the setters because all validation happens inside those constructors.This PR proposes to remove those setters and introduce a constructor instead that allows to set both properties once.