fix: update threshold handling to support float values in all usages#528
fix: update threshold handling to support float values in all usages#528mariusjp wants to merge 6 commits intoericsizemore:masterfrom
Conversation
|
Thanks for starting on this. I'll review it once I am able. :) Looks like a good start. |
|
The only part I don't really like is: ->setThreshold(Utils::convertThresholdToFloat($threshold)) // (CoverageCheckCommand.php:162)It feels like I should not use Utils here because now I also have to Cover this part in the UnitTest. What's your take on this? |
|
My apologies Marius, been under the weather with COVID. I promise I'll take a look within the next few days. It's kicking my butt right now. :) |
|
No worries, get well soon! |
ericsizemore
added
enhancement
| final class ThresholdOutOfBoundsException extends InvalidArgumentException | ||
| { | ||
| public static function create(int $threshold): self | ||
| public static function create(float $threshold): self |
There was a problem hiding this comment.
Instead of moving from int to float, perhaps we should just expand it to float|int.
| private bool $onlyPercentage = false; | ||
|
|
||
| private int $threshold = 100; | ||
| private float $threshold = 100; |
There was a problem hiding this comment.
Instead of moving from int to float, perhaps we should just expand it to float|int.
| } | ||
|
|
||
| public function getThreshold(): int | ||
| public function getThreshold(): float |
There was a problem hiding this comment.
Instead of moving from int to float, perhaps we should just expand it to float|int.
| * defined range (>= 1 <= 100). | ||
| */ | ||
| public function nonConsoleCall(string $cloverFile, int $threshold = 100, bool $onlyPercentage = false): string | ||
| public function nonConsoleCall(string $cloverFile, float $threshold = 100, bool $onlyPercentage = false): string |
There was a problem hiding this comment.
Instead of moving from int to float, perhaps we should just expand it to float|int.
| * @throws ThresholdOutOfBoundsException If the threshold is less than 1 or greater than 100. | ||
| */ | ||
| public function setThreshold(int $threshold): CoverageCheck | ||
| public function setThreshold(float $threshold): CoverageCheck |
There was a problem hiding this comment.
Instead of moving from int to float, perhaps we should just expand it to float|int.
| * A simple check to determine if threshold is within accepted range (Min. 1, Max. 100). | ||
| */ | ||
| public static function validateThreshold(int $threshold): bool | ||
| public static function validateThreshold(float $threshold): bool |
There was a problem hiding this comment.
Kind of tied in with previous commentds, instead of moving from int to float, perhaps we should just expand it to float|int.
| return ($cloverFile !== '' && file_exists($cloverFile)); | ||
| } | ||
|
|
||
| public static function convertThresholdToFloat(string $threshold): float |
There was a problem hiding this comment.
Should we look at potentially a different method? Perhaps instead of converting to, and returning as, float - we could check the input itself to see if it's "floaty" or "inty" and return either float|int?
|
Gone through and made some comments, just some initial thoughts. I'm still a bit out of it, so my thinking could be a tad off haha. ETA: Wondering if this should also be pushed to a new major version. Though that could depend on how we move forward I suppose, with the type safety changes. |
|
To respond to basically all your comments. |
|
Just noticed some of my auto formatting changed the order of your docblocks. I'll fix those asap. I'll also try to figure out how to get below that 3% threshold |
|
|
Appreciate all your work on this Marius. My apologies I haven't been able to be more involved with this. A lot going on at the moment, but hope I'll be able to make some time soon. |
2 participants
Pull Request
Proposed Changes
Readiness Checklist
Author/Contributor
composer run-script phpunitand ensure you have test coverage for the lines you are introducingcomposer run-script phpstanand fix any issues that you have introducedcomposer run-script psalmand fix any issues that you have introducedcomposer run-script phpcs:checkand fix any issues that you have introducedReviewer
fix,documentation, orenhancementverifiedorunverified