-
Notifications
You must be signed in to change notification settings - Fork 736
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
Bump PHPStan to level 5 #2108
Bump PHPStan to level 5 #2108
Conversation
tests/ClientFunctionalTest.php
Outdated
$ixCoin->setIndex(null); // Make sure the index gets set properly if missing | ||
$index->deleteDocuments([$anonCoin, $ixCoin]); | ||
$index->deleteDocuments([ | ||
new Document('1', ['name' => 'AnonCoin']), |
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.
Why do you need to recreate the doc here for this change? I also wander why we do it onl line 170/171?
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.
I've updated the test to reuse the documents and added a couple of tests to check that the index is set if missing (to remove the $ixCoin->setIndex(null);
part)
@@ -30,6 +30,16 @@ parameters: | |||
count: 1 | |||
path: src/Transport/Guzzle.php | |||
|
|||
- |
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.
Trying to get my head around why we don't fix these in the test files directly? Related to breaking changes?
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.
This is what it was explained in the PR description #2108 (comment), a test like this:
Elastica/tests/Aggregation/GeohashGridTest.php
Lines 51 to 57 in 8fe6176
public function testGeohashGridAggregationWithNotAllowedPrecision(): void | |
{ | |
$this->expectException(\TypeError::class); | |
$agg = new GeohashGrid('hash', 'location'); | |
$agg->setPrecision(1.5); | |
} |
makes PHPStan fails because setPrecision
only accepts int|string
:
Elastica/src/Aggregation/GeohashGrid.php
Lines 42 to 50 in 6d51cdb
* @param int|string $precision an integer between 1 and 12, inclusive. Defaults to 5 or distance like 1km, 10m | |
* | |
* @return $this | |
*/ | |
public function setPrecision($precision): self | |
{ | |
if (!\is_int($precision) && !\is_string($precision)) { | |
throw new \TypeError(\sprintf('Argument 1 passed to "%s()" must be of type int|string, %s given.', __METHOD__, \is_object($precision) ? \get_class($precision) : \gettype($precision))); | |
} |
We can get rid of these ignored errors and checks in the next major if we add type parameter declarations.
@franmomu Thanks for getting us to phpstan level 5 |
Most of the ignored errors are expected because they are testing exceptions using wrong types and PHPStan complains about that. Like:
Elastica/tests/Aggregation/GeohashGridTest.php
Lines 51 to 57 in 8fe6176
The different ones are:
tests/Query/FunctionScoreTest.php
, which I think it's fine to addTODO
just in case someone has overriddenFunctionScore::addDecayFunction()
method, if this is fine we can close Allowint
andfloat
forFunctionScore::addDecayFunction()
origin and scale #2089ClientFunctionTest
, there is a call toDocument::setIndex()
passingnull
which is not allowed, that is done to make sure that the index is properly set when deleting the documents. Instead of that a new document can be passed as we do lines above to update them:Elastica/tests/ClientFunctionalTest.php
Lines 161 to 182 in 8fe6176