Skip to content

Add ResultInterface template #67

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.

Sign up for GitHub

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

Closed
bram123 wants to merge 2 commits into from
Closed

Add ResultInterface template #67

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ad0b5d
Add ResultInterface template for the `data` field, for strict typing …
bram123 Jan 5, 2023
70d518f
Merge branch 'laminas:1.25.x' into result_data_template
bram123 Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/Check/DiskFree.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
* authors: Dave Ingram <dave@dmi.me.uk>, Nick Pope <nick@nickpope.me.uk>
* license: http://creativecommons.org/licenses/BSD/ CC-BSD
* copyright: Copyright (c) 2010, Dave Ingram, Nick Pope
*
* @psalm-type ResultData array{availability: array{value: float, valueType: string}}
*/
class DiskFree extends AbstractCheck implements CheckInterface
Comment on lines +35 to 37
Copy link
Copy Markdown
Member

@Ocramius Ocramius Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think CheckInterface should specify the psalm template here: not each concrete implementation.

Declaring an @template TResult of Failure|Success|Warning on the interface should highlight the missing @template-implements in each implementation of CheckInterface, and then we can remove all manual @return declarations on each of the check() methods.

WDYT?

Copy link
Copy Markdown
Contributor Author

@bram123 bram123 Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius
That means that other code repositories with custom implementations of the interface, would also have failing psalm/phpstan checks after updating right?
It would be the better solution yes, and mean that all new checks would also set this up.
If the static-analysis issue is okay (don't know if it falls under a bc break, as the code api doesnt change) I will change this :)

Copy link
Copy Markdown
Member

@Ocramius Ocramius Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they would get a failure during analysis, but we don't really consider those BC breaks: the code works as it did before, and the types are just more precise.

Copy link
Copy Markdown
Contributor Author

@bram123 bram123 Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius
Did a small test, and the current psalm version (4.30) does not error when I add a template to CheckInterface. Updating locally to psalm 5.6 does trigger MissingTemplateParam errors, but I see that update is being worked on in #62

Copy link
Copy Markdown
Member

@Ocramius Ocramius Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave #62 alone for now :)

As for the patch: please rebase, instead of merging upstream.

{
Expand Down Expand Up @@ -168,7 +170,7 @@ public function __construct($size, $path = '/')
*
* @see \Laminas\Diagnostics\Check\CheckInterface::check()
*
* @return Failure|Success|Warning
* @return Failure<ResultData>|Success<ResultData>|Warning
*/
public function check()
{
Expand All @@ -186,10 +188,10 @@ public function check()
$description = sprintf('Remaining space at %s: %s', $this->path, $freeHumanReadable);

if (disk_free_space($this->path) < $this->minDiskBytes) {
return new Failure($description, $free);
return new Failure($description, ['availability' => ['value' => $free, 'valueType' => 'bytes']]);
}

return new Success($description, $free);
return new Success($description, ['availability' => ['value' => $free, 'valueType' => 'bytes']]);
}

/**
Expand Down
19 changes: 15 additions & 4 deletions src/Check/DiskUsage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

/**
* Checks to see if the disk usage is below warning/critical percent thresholds
*
* @psalm-type ResultData array{utilization: array{value: float, valueType: string}}
*/
class DiskUsage extends AbstractCheck implements CheckInterface
{
Expand Down Expand Up @@ -85,7 +87,7 @@ public function __construct($warningThreshold, $criticalThreshold, $path = '/')
/**
* Perform the check
*
* @return Failure|Success|Warning
* @return Failure<ResultData>|Success<ResultData>|Warning<ResultData>
*/
public function check()
{
Expand All @@ -95,13 +97,22 @@ public function check()
$dp = ($du / $dt) * 100;

if ($dp >= $this->criticalThreshold) {
return new Failure(sprintf('Disk usage too high: %2d percent.', $dp), $dp);
return new Failure(
sprintf('Disk usage too high: %2d percent.', $dp),
['utilization' => ['value' => $dp, 'valueType' => 'percentage']]
);
}

if ($dp >= $this->warningThreshold) {
return new Warning(sprintf('Disk usage high: %2d percent.', $dp), $dp);
return new Warning(
sprintf('Disk usage high: %2d percent.', $dp),
['utilization' => ['value' => $dp, 'valueType' => 'percentage']]
);
}

return new Success(sprintf('Disk usage is %2d percent.', $dp), $dp);
return new Success(
sprintf('Disk usage is %2d percent.', $dp),
['utilization' => ['value' => $dp, 'valueType' => 'percentage']]
);
}
}
12 changes: 7 additions & 5 deletions src/Result/AbstractResult.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,22 @@

/**
* Abstract, simple implementation of ResultInterface
*
* @template T
*/
abstract class AbstractResult implements ResultInterface
{
/** @var string */
protected $message;

/** @var mixed|null */
/** @var T|null */
protected $data;

/**
* Create new result
*
* @param string $message
* @param mixed|null $data
* @param string $message
* @param T|null $data
*/
public function __construct($message = '', $data = null)
{
Expand All @@ -41,15 +43,15 @@ public function getMessage()
/**
* Get detailed info on the test result (if available).
*
* @return mixed|null
* @return T|null
*/
public function getData()
{
return $this->data;
}

/**
* @param mixed|null $data
* @param T|null $data
*/
public function setData($data)
{
Expand Down
5 changes: 5 additions & 0 deletions src/Result/Failure.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
* @extends AbstractResult<T>
* @implements FailureInterface<T>
*/
class Failure extends AbstractResult implements FailureInterface
{
}
4 changes: 4 additions & 0 deletions src/Result/FailureInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
* @implements ResultInterface<T>
*/
interface FailureInterface extends ResultInterface
{
}
5 changes: 4 additions & 1 deletion src/Result/ResultInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
*/
interface ResultInterface
{
/**
Expand All @@ -14,7 +17,7 @@ public function getMessage();
/**
* Get detailed info on the test result (if available).
*
* @return mixed|null
* @return T|null
*/
public function getData();
}
5 changes: 5 additions & 0 deletions src/Result/Skip.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
* @extends AbstractResult<T>
* @implements SkipInterface<T>
*/
class Skip extends AbstractResult implements SkipInterface
{
}
4 changes: 4 additions & 0 deletions src/Result/SkipInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
* @implements ResultInterface<T>
*/
interface SkipInterface extends ResultInterface
{
}
5 changes: 5 additions & 0 deletions src/Result/Success.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
* @extends AbstractResult<T>
* @implements SuccessInterface<T>
*/
class Success extends AbstractResult implements SuccessInterface
{
}
4 changes: 4 additions & 0 deletions src/Result/SuccessInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
* @implements ResultInterface<T>
*/
interface SuccessInterface extends ResultInterface
{
}
5 changes: 5 additions & 0 deletions src/Result/Warning.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
* @extends AbstractResult<T>
* @implements WarningInterface<T>
*/
class Warning extends AbstractResult implements WarningInterface
{
}
4 changes: 4 additions & 0 deletions src/Result/WarningInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

namespace Laminas\Diagnostics\Result;

/**
* @template T
* @implements ResultInterface<T>
*/
interface WarningInterface extends ResultInterface
{
}
1 change: 1 addition & 0 deletions test/BasicClassesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public function testConstructor(): void

public function testSetters(): void
{
/** @var Success<mixed> $result */
$result = new Success();

self::assertSame('', $result->getMessage());
Expand Down
30 changes: 21 additions & 9 deletions test/DiskFreeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@

use InvalidArgumentException;
use Laminas\Diagnostics\Check\DiskFree;
use Laminas\Diagnostics\Result\FailureInterface;
use Laminas\Diagnostics\Result\SuccessInterface;
use Laminas\Diagnostics\Result\WarningInterface;
use Laminas\Diagnostics\Result\Failure;
use Laminas\Diagnostics\Result\Success;
use Laminas\Diagnostics\Result\Warning;
use PHPUnit\Framework\TestCase;

use function count;
Expand Down Expand Up @@ -142,13 +142,21 @@ public function testJitFreeSpace(): void
$check = new DiskFree($freeRightNow * 0.5, $tmp);
$result = $check->check();

self::assertInstanceof(SuccessInterface::class, $result);
self::assertInstanceof(Success::class, $result);
$data = $result->getData();
self::assertNotNull($data);
self::assertSame($freeRightNow, $data['availability']['value']);
self::assertSame('bytes', $data['availability']['valueType']);

$freeRightNow = disk_free_space($tmp);
$check = new DiskFree($freeRightNow + 1073741824, $tmp);
$result = $check->check();

self::assertInstanceof(FailureInterface::class, $result);
self::assertInstanceof(Failure::class, $result);
$data = $result->getData();
self::assertNotNull($data);
self::assertSame($freeRightNow, $data['availability']['value']);
self::assertSame('bytes', $data['availability']['valueType']);
}

public function testSpaceWithStringConversion(): void
Expand All @@ -160,20 +168,24 @@ public function testSpaceWithStringConversion(): void
}

// give some margin of error
$freeRightNow *= 0.9;
$freeRightNowString = DiskFree::bytesToString($freeRightNow);
$freeRightNowString = DiskFree::bytesToString((int) ($freeRightNow * 0.9));
$check = new DiskFree($freeRightNowString, $tmp);
$result = $check->check();

self::assertInstanceof(SuccessInterface::class, $result);
self::assertInstanceof(Success::class, $result);
$data = $result->getData();
self::assertNotNull($data);
self::assertSame($freeRightNow, $data['availability']['value']);
self::assertSame('bytes', $data['availability']['valueType']);
}

public function testInvalidPathShouldReturnWarning(): void
{
$check = new DiskFree(1024, __DIR__ . '/someImprobablePath99999999999999999');
$result = $check->check();

self::assertInstanceof(WarningInterface::class, $result);
self::assertInstanceof(Warning::class, $result);
self::assertNull($result->getData());
}

public function testInvalidSizeParamThrowsException(): void
Expand Down
15 changes: 12 additions & 3 deletions test/DiskUsageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,28 @@ public function testCheck(): void
$result = $check->check();

self::assertInstanceof(SuccessInterface::class, $result);
self::assertSame($dp, $result->getData());
$data = $result->getData();
self::assertNotNull($data);
self::assertSame($dp, $data['utilization']['value']);
self::assertSame('percentage', $data['utilization']['valueType']);

$check = new DiskUsage($dp - 1, 100, $this->getTempDir());
$result = $check->check();

self::assertInstanceof(WarningInterface::class, $result);
self::assertSame($dp, $result->getData());
$data = $result->getData();
self::assertNotNull($data);
self::assertSame($dp, $data['utilization']['value']);
self::assertSame('percentage', $data['utilization']['valueType']);

$check = new DiskUsage(0, $dp - 1, $this->getTempDir());
$result = $check->check();

self::assertInstanceof(FailureInterface::class, $result);
self::assertSame($dp, $result->getData());
$data = $result->getData();
self::assertNotNull($data);
self::assertSame($dp, $data['utilization']['value']);
self::assertSame('percentage', $data['utilization']['valueType']);
}

public function invalidArgumentProvider(): array
Expand Down