Request coverage reports with command line args #1375
Conversation
|
|
||
| $this->tmpDir = $tmpDir; | ||
| $this->jUnitFilePath = $jUnitFilePath; | ||
| } |
There was a problem hiding this comment.
Extending constructors does not break the LSP.
| @@ -52,7 +52,7 @@ public function build(string $configPath, string $extraOptions): array | |||
| '--configuration', | |||
| $configPath, | |||
| ], | |||
| explode(' ', $extraOptions) | |||
| explode(' ', $extraOptions) // FIXME might break space-containing paths | |||
There was a problem hiding this comment.
Not fixing this issue in this PR, long enough already.
tests/phpunit/TestFramework/PhpUnit/Config/Builder/InitialConfigBuilderTest.php
Outdated
Show resolved
Hide resolved
…igBuilderTest.php
| array $phpExtraArgs, | ||
| bool $skipCoverage | ||
| ): array { | ||
| if ($skipCoverage === false) { |
There was a problem hiding this comment.
question: is it any better than !$skiptCoverage? Found a couple of places and all ones from you. I mean we usually use !$condition with booleans, probably worth unifying
There was a problem hiding this comment.
For one, !$condition is messy, it might trigger on a null. For another, use === is more generic to all types, more uniform. What do you think?
Is there a fixer for !$bool?
There was a problem hiding this comment.
I was very hesitant about === if you remember, and I'm still not exactly on board with the arguments I've seen back then. True, it can be ugly when comparing strings and numbers, for example, but the real reason I'm using === everywhere effectively out of habit now because it prevents unnecessary and avoidable BC breaks.
It is not like someone would extend the very this class, but if this happens, identical comparison gets us prepared. For example, if you were using !$condition and someone extending extending this class decides put a different falsy value here, you can't really change it to something else without a BC break. Say, you want to give a null special meaning? Well, you can't, because null is falsy. On the contrary, if you use $condition === false, you're protected from said BC breaks. Add any nulls anytime you want! That's the reason. Makes sense?
There was a problem hiding this comment.
makes sense
I know the reason why it looks weird for me, because I've been using phpstan with strict rules for 2 years on my job, and it's impossible to use non-boolean comparison there.
Look at this code with strict rules:
$null = rand(0, 1) === 0 ? null : true;
if (!$null) {
}error: Only booleans are allowed in a negated boolean, true|null given.. So if it's nullable - makes sense to use === true, but when it's bool only - it's safe to use just !$null here.
Regarding your example, I guess it's impossible to do it: https://phpstan.org/r/b07764ac-22c8-4c06-ad83-a19ba9e09381 - we can pass only booleans to the parent (original) method.
Anyway, it's not critical, we can leave it. Just not usual usage for me, that's why I asked.
There was a problem hiding this comment.
Sure. It's nice to have PHPStan warn you about improper negation, but it's a liability nevertheless. E.g. where negation can be improper, === is always safe to use.
This PR:
What left to do with #1283:
addCoverageFilterWhitelistIfDoesNotExist).