Skip to content
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

Added different message to distinguish between two cases: #495

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Added different message to distinguish between two cases: #495

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93af1e7
Added different message to distinguish between two cases:
Sep 25, 2024
6b02ca3
Moved exception message generation to named constructors
zerkms Oct 28, 2024
8915096
Fix nested missing values
zerkms Nov 6, 2024
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
32 changes: 26 additions & 6 deletions src/Psl/Type/Exception/AssertException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,28 @@ final class AssertException extends Exception
/**
* @param list<string> $paths
*/
private function __construct(string $actual, string $expected, array $paths = [], ?Throwable $previous = null)
private function __construct(?string $actual, string $expected, array $paths = [], ?Throwable $previous = null)
{
$first = $previous instanceof Exception ? $previous->getFirstFailingActualType() : $actual;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this be "null" on nested shapes where one of the underlying keys does not exist? (given that you pass null as a string to the parent exception?

(same comment for CoercionException)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what it means, could you expand? If the "outer" shape does not match - no "nested" shapes will be checked, right?

Copy link
Collaborator

@veewee veewee Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For example:

shape([
    'a' => shape([
        'b' => mixed()
    ]),
])->coerce(['a' => []]);

This will result in:

Could not coerce "null" to type "array{'a': array{'b': mixed}}" at path "a.b"
 -> Previous:  Could not coerce to type "array{'b': mixed}" at path "b" as the value was not passed.

If the "outer" shape does not match - no "nested" shapes will be checked, right?

The types will coerce top-down indeed. But if one of the nested children results in a failure, the exception gets wrapped and enriched with the "path" information. It's that $first logic inside the exception constructor which you default to ?? 'null'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, now I see that CoercionException and AssertException should be monadic. How about now?

Only for CoercionException yet, as a proof-of-concept.

And a mandatory reminder: this is still under heavy WIP, I understand it's not a trivial change and I will clean things up during the final git squash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think wrap()-ing it is the way:

vec(
    shape([
    'a' => shape(['b' => mixed()])
    ])
)->coerce([['a' => []]]);

Could not coerce "null" to type "vec<array{'a': array{'b': mixed}}>" at path "0.a.b"

The parent's exception construction should be more aware of those details from within the child's type exception.
Maybe an alternate suggestion would to make the $actual and $first property nullable for those situations where the value is not available and let that decide the message instead?


parent::__construct(
Str\format(
if ($first !== null) {
$message = Str\format(
'Expected "%s", got "%s"%s.',
$expected,
$first,
$paths ? ' at path "' . Str\join($paths, '.') . '"' : ''
),
$actual,
$paths ? ' at path "' . Str\join($paths, '.') . '"' : '',
);
} else {
$message = Str\format(
zerkms marked this conversation as resolved.
Show resolved Hide resolved
'Expected "%s", received no value at path "%s".',
$expected,
Str\join($paths, '.'),
);
}

parent::__construct(
$message,
$actual ?? 'null',
$paths,
$previous
);
Expand All @@ -51,4 +61,14 @@ public static function withValue(

return new self(get_debug_type($value), $expected_type, Vec\filter_nulls($paths), $previous);
}

public static function withoutValue(
string $expected_type,
?string $path = null,
?Throwable $previous = null
): self {
$paths = $previous instanceof Exception ? [$path, ...$previous->getPaths()] : [$path];

return new self(null, $expected_type, Vec\filter_nulls($paths), $previous);
}
}
31 changes: 26 additions & 5 deletions src/Psl/Type/Exception/CoercionException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,30 @@ final class CoercionException extends Exception
/**
* @param list<string> $paths
*/
private function __construct(string $actual, string $target, array $paths = [], ?Throwable $previous = null)
private function __construct(?string $actual, string $target, array $paths = [], ?Throwable $previous = null)
{
$first = $previous instanceof Exception ? $previous->getFirstFailingActualType() : $actual;

parent::__construct(
Str\format(
if ($first !== null) {
$message = Str\format(
'Could not coerce "%s" to type "%s"%s%s.',
$first,
$target,
$paths ? ' at path "' . Str\join($paths, '.') . '"' : '',
$previous && !$previous instanceof self ? ': ' . $previous->getMessage() : '',
),
$actual,
);
} else {
$message = Str\format(
'Could not coerce to type "%s" at path "%s" as the value was not passed%s.',
$target,
Str\join($paths, '.'),
$previous && !$previous instanceof self ? ': ' . $previous->getMessage() : '',
);
}

parent::__construct(
$message,
$actual ?? 'null',
$paths,
$previous
);
Expand All @@ -52,4 +63,14 @@ public static function withValue(

return new self(get_debug_type($value), $target, Vec\filter_nulls($paths), $previous);
}

public static function withoutValue(
string $target,
?string $path = null,
?Throwable $previous = null
): self {
$paths = $previous instanceof Exception ? [$path, ...$previous->getPaths()] : [$path];

return new self(null, $target, Vec\filter_nulls($paths), $previous);
}
}
4 changes: 2 additions & 2 deletions src/Psl/Type/Internal/ShapeType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ private function coerceIterable(mixed $value): array
continue;
}

throw CoercionException::withValue(null, $this->toString(), PathExpression::path($element));
throw CoercionException::withoutValue($this->toString(), PathExpression::path($element));
}
} catch (CoercionException $e) {
throw match (true) {
Expand Down Expand Up @@ -196,7 +196,7 @@ public function assert(mixed $value): array
continue;
}

throw AssertException::withValue(null, $this->toString(), PathExpression::path($element));
throw AssertException::withoutValue($this->toString(), PathExpression::path($element));
}
} catch (AssertException $e) {
throw match (true) {
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/Type/ShapeTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ public static function provideAssertExceptionExpectations(): iterable
'name' => Type\string(),
]),
[],
'Expected "array{\'name\': string}", got "null" at path "name".'
'Expected "array{\'name\': string}", received no value at path "name".'
];
yield 'invalid key' => [
Type\shape([
Expand Down Expand Up @@ -247,7 +247,7 @@ public static function provideCoerceExceptionExpectations(): iterable
'name' => Type\string(),
]),
[],
'Could not coerce "null" to type "array{\'name\': string}" at path "name".'
'Could not coerce to type "array{\'name\': string}" at path "name" as the value was not passed.'
];
yield 'invalid key' => [
Type\shape([
Expand Down Expand Up @@ -295,7 +295,7 @@ public static function provideCoerceExceptionExpectations(): iterable
(static function () {
yield null => 'nope';
})(),
'Could not coerce "null" to type "array{\'id\': int}" at path "id".'
'Could not coerce to type "array{\'id\': int}" at path "id" as the value was not passed.'
];
yield 'iterator yielding object key' => [
Type\shape([
Expand All @@ -305,7 +305,7 @@ public static function provideCoerceExceptionExpectations(): iterable
yield (new class () {
}) => 'nope';
})(),
'Could not coerce "null" to type "array{\'id\': int}" at path "id".'
'Could not coerce to type "array{\'id\': int}" at path "id" as the value was not passed.'
];
}

Expand Down