[Finder] Make Comparator immutable #42137
Conversation
| @@ -73,7 +73,6 @@ public function __construct(?string $test) | |||
| } | |||
| } | |||
|
|
|||
| $this->setTarget($target); | |||
| $this->setOperator($matches[1] ?? '=='); | |||
| parent::__construct($target, $matches[1] ?: '=='); | |||
There was a problem hiding this comment.
did you change ?? to ?: on purpose?
There was a problem hiding this comment.
Yes. setOperator() accepted an empty string and I've made the constructor stricter in that regard.
c937eec to
4f0089b
Compare
| @@ -26,14 +32,22 @@ public function testGetSetOperator() | |||
| $this->assertInstanceOf(\InvalidArgumentException::class, $e, '->setOperator() throws an \InvalidArgumentException if the operator is not valid.'); | |||
| } | |||
|
|
|||
| $comparator = new Comparator(); | |||
| $comparator = new Comparator('some target'); | |||
There was a problem hiding this comment.
We have 2 tests in one here. Those should be split (which will also allow using expectException 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.
| @@ -43,9 +57,10 @@ public function testGetSetTarget() | |||
| */ | |||
| public function testTest($operator, $target, $match, $noMatch) | |||
There was a problem hiding this comment.
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.
The tests for this class are indeed pretty sparse. I think the class is mainly tested by testing its subclasses.
4f0089b to
0ff3971
Compare
This 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 necessary Commits ------- 4c44dce [Finder] Improve ComparatorTest
Signed-off-by: Alexander M. Turek <me@derrabus.de>
|
Thank you @derrabus. |
0ff3971 to
b83ead2
Compare
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.