Repository navigation
Conversation
darwinz
left a comment
darwinz
left a comment
There was a problem hiding this comment.
Thanks for your contribution! Please run the linter by running vendor/bin/phpcs -n. There are a few linter errors that need to be addressed. Thanks!
Co-authored-by: Brandon Johnson <bbj1979@gmail.com>
|
@haj8110, please run the linter on your branch and fix the errors. |
|
@haj8110 I can't merge until you get the linter to pass. Are you able to fix the errors? |
… code style - Updated class definitions to follow PSR standards by adding opening braces on the same line. - Adjusted method definitions to align with coding conventions. - Ensured consistent formatting and spacing throughout the conversion methods.
darwinz
left a comment
darwinz
left a comment
There was a problem hiding this comment.
Thanks for fixing the lint errors, phpcs -n passes now!
I merged this against current master locally and ran composer test: 27 tests, 8 failures. All of the LengthConversions tests fail on the exception message. I've left inline comments with the details; the main items are the failing tests, the assertion tolerance, the inconsistent mile constants, and matching the repo conventions from the PHP 8.2 modernization (strict types, typed signatures, file names).
You can check locally with composer test and vendor/bin/phpcs -n. Thanks!
| @@ -0,0 +1,118 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Since the PHP 8.2 modernization (#190), every file in Conversions/ starts with declare(strict_types=1);. Please add it here, and add parameter/return types to the methods (e.g. public static function mToKm(float|int $meters): float).
Also, please rename this file to LengthConversions.php so it matches the class name and the existing naming (TemperatureConversions.php, SpeedConversion.php).
| { | ||
| // Make sure we have a numeric value | ||
| if (!is_numeric($value)) { | ||
| throw new InvalidArgumentException("Invalid input for $method: expected numeric, got " . gettype($value) . " (" . var_export($value, true) . ")"); |
There was a problem hiding this comment.
This message doesn't match what the tests expect ('Invalid input for mToKm'), so all 8 LengthConversions tests fail when I run composer test against master. Please use the same message as WeightConversions::validateInput.
| public static function mToMiles($meters) | ||
| { | ||
| self::validateInput($meters, 'mToMiles'); | ||
| return round($meters / 1609.34, 6); |
There was a problem hiding this comment.
The mile constant here (1609.34 m) disagrees with the one in kmToMiles/milesToKm (1.609 km), so mToMiles(1000) returns 0.621373 while kmToMiles(1) returns 0.621504. The exact value is 1609.344 m. Could you define it once as a class constant (e.g. private const METERS_PER_MILE = 1609.344;) and use it in all four mile conversions?
| public static function cmToIn($centimeters) | ||
| { | ||
| self::validateInput($centimeters, 'cmToIn'); | ||
| return round($centimeters / 2.54, 2); |
There was a problem hiding this comment.
Rounding to 2 decimals loses a lot of precision (cmToIn(1) returns 0.39). The precision also varies across methods (2, 4, 5, 6). I'd suggest dropping the round() calls and letting the tests compare with a delta, or at least using one precision everywhere.
| public static function kmToMiles($km) | ||
| { | ||
| self::validateInput($km, 'kmToMiles'); | ||
| return round($km / 1.609, 5); |
There was a problem hiding this comment.
1.609 should be 1.609344 (see the comment on mToMiles).
| @@ -0,0 +1,96 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Same as the length file: please add declare(strict_types=1); and parameter/return types, and rename the file to WeightConversions.php to match the class name.
| * @param mixed $value The invalid input value | ||
| * @param string $expectedMessage The expected exception message | ||
| */ | ||
| protected function assertInvalidInputConversion($method, $value, $expectedMessage) |
There was a problem hiding this comment.
This helper is more machinery than we need. The rest of this file uses $this->expectException(...) / $this->expectExceptionMessage(...), so a separate small test per invalid-input case would be more consistent.
| } | ||
| public function testKgToLbs() | ||
| { | ||
| $this->assertEquals(220.462, WeightConversions::kgToLbs(100), 0.001); |
There was a problem hiding this comment.
In PHPUnit 10/11 the third argument of assertEquals is the failure message, not a delta, so 0.001 isn't a tolerance here. These are exact float comparisons that only pass because the rounded outputs happen to match. Please switch these to assertEqualsWithDelta($expected, $actual, 0.001) (this applies to every conversion assertion below too).
| * [Octaltodecimal](./Conversions/OctalToDecimal.php) | ||
| * [Speedconversion](./Conversions/SpeedConversion.php) | ||
| * [Temperatureconversions](./Conversions/TemperatureConversions.php) | ||
| * [Weightconversions](./Conversions/Weightconversions.php) |
There was a problem hiding this comment.
Please drop the DIRECTORY.md changes. That file is regenerated automatically by the directory workflow after merge.
darwinz
left a comment
darwinz
left a comment
There was a problem hiding this comment.
Marking this as changes requested so it isn't merged before the items in my review above are addressed. Please see the inline comments there.
No description provided.