-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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 TryCatchStrategy #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good, just a few small changes to the code. However, the documentation must also be updated before this can be accepted. Please add TryCatch to the strategy section of the README.md
file.
new TryCatch( | ||
$this->callback, | ||
function ($e) { | ||
if (get_class($e) === \DomainException::class) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an instanceof
check instead of a string comparison, e.g. $e instanceof DomainException
.
{ | ||
/** @var TryCatch $tryCatch */ | ||
$tryCatch = ( | ||
new TryCatch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line (and everything nested below it) should be indented one level more.
'DomainExceptionHandled' | ||
) | ||
)->setMapper(new Mapper); | ||
self::assertSame(['foo','bar'], $tryCatch(['foo','bar'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Prefer blank line before assertion block; here and in the previous test.
- Should be a space after each comma (
,
) in array literal. Applies throughout this file.
return $data; | ||
}); | ||
} | ||
public function testTryCatch() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be blank line before starting new method.
public function __construct(Strategy $strategy, callable $handler, $expression) | ||
{ | ||
parent::__construct($strategy); | ||
$this->handler = $handler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer blank line after parent call.
new TryCatch( | ||
new TryCatch( | ||
$this->callback, | ||
function ($e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be type hinted, e.g. Exception $e
.
#### Signature | ||
|
||
```php | ||
ToList(Strategy $strategy, callable $handler, Strategy|Mapping|array|mixed $expression) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like ToList should actually be TryCatch here.
1 similar comment
Merged at #️⃣ 6d21f83. Thanks @a-barzanti for your great work! |
No description provided.