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

[WIP] Addition of `Exception` deriving #77

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@gquemener
Contributor

gquemener commented Jul 20, 2018

See #55 and #58 for reference.

Todo

  • Add basic exception definition
  • Add support for custom base exception class
  • Add support for named constructor with args
  • Add support for named constructor without args
  • Add support for default exception message
  • Short error message syntax dumping
  • Add error when parent class is not throwable
  • Write documentation
@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Jul 20, 2018

Owner

Is this already finished for review? Tests are failing.

Owner

prolic commented Jul 20, 2018

Is this already finished for review? Tests are failing.

@gquemener

This comment has been minimized.

Show comment
Hide comment
@gquemener

gquemener Jul 20, 2018

Contributor

Sorry, nop it's WIP

Contributor

gquemener commented Jul 20, 2018

Sorry, nop it's WIP

@gquemener gquemener changed the title from Addition of `Exception` deriving to [WIP] Addition of `Exception` deriving Jul 20, 2018

@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Jul 20, 2018

Owner

See also DumpTest::it_throws_exception_when_a_marker_extends_an_exception_type

Owner

prolic commented Jul 20, 2018

See also DumpTest::it_throws_exception_when_a_marker_extends_an_exception_type

gquemener added some commits Jul 20, 2018

@gquemener

This comment has been minimized.

Show comment
Hide comment
@gquemener

gquemener Jul 25, 2018

Contributor

Regarding DumpTest::it_throws_exception_when_a_marker_extends_an_exception_type, I've simply removed the test as there will be no exception type with this PR.

Contributor

gquemener commented Jul 25, 2018

Regarding DumpTest::it_throws_exception_when_a_marker_extends_an_exception_type, I've simply removed the test as there will be no exception type with this PR.

@gquemener

This comment has been minimized.

Show comment
Hide comment
@gquemener

gquemener Jul 25, 2018

Contributor

Hey @prolic, I'm thinking about implementing message conversion, aka

'User {{$user}} is not available' => sprintf('User %s is not available', $user->toString())

You mentionned here that something is already implemented and that it could be reused. Could you point me to which part of the lib you were making reference please?

Contributor

gquemener commented Jul 25, 2018

Hey @prolic, I'm thinking about implementing message conversion, aka

'User {{$user}} is not available' => sprintf('User %s is not available', $user->toString())

You mentionned here that something is already implemented and that it could be reused. Could you point me to which part of the lib you were making reference please?

@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Jul 25, 2018

Owner

@gquemener See f.e. buildToArrayBody function.

Owner

prolic commented Jul 25, 2018

@gquemener See f.e. buildToArrayBody function.

@@ -57,4 +57,9 @@ public function isScalarTypeHint(): bool
{
return \in_array($this->type, ['string', 'int', 'bool', 'float'], true);
}
public function __toString(): string

This comment has been minimized.

@gquemener

gquemener Jul 25, 2018

Contributor

This is usefull to make the "union" operation (array_unique(array_merge())) work (see buildExceptionConstructors)

@gquemener

gquemener Jul 25, 2018

Contributor

This is usefull to make the "union" operation (array_unique(array_merge())) work (see buildExceptionConstructors)

gquemener added some commits Jul 25, 2018

@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Aug 25, 2018

Owner

@gquemener It's been a while since you were working on this. Still on it? No hurries, just asking.

Owner

prolic commented Aug 25, 2018

@gquemener It's been a while since you were working on this. Still on it? No hurries, just asking.

@prolic prolic added the enhancement label Aug 25, 2018

@gquemener

This comment has been minimized.

Show comment
Hide comment
@gquemener

gquemener Aug 25, 2018

Contributor

Hello,

I've taken some off time in August, I will get back on this PR in September.
I'll give you a full status report at this moment.

Cheers 👋

Contributor

gquemener commented Aug 25, 2018

Hello,

I've taken some off time in August, I will get back on this PR in September.
I'll give you a full status report at this moment.

Cheers 👋

@gquemener

This comment has been minimized.

Show comment
Hide comment
@gquemener

gquemener Sep 5, 2018

Contributor

Hello,

I've had some time to review the state of this PR.

Apart from the short message syntax and documentation, everything else has been implemented (which, I believe, already makes the feature usable).
My suggestion, mainly because I don't have time to take care on the syntax part, would be to focus on the documentation and merge it.

However, @prolic has added a default value feature which impacts this PR.
As a matter of fact, I've extracted the data constructor arguments parsing logic in a reusable function, in order to reuse it for the exception constructor arguments parsing.
The rebasing is not trivial and makes me think about conflict around the exception's ctor $message argument which also has a default value (defined through the _ => 'foo' syntax AFAIR).

Would you mind handling/trying this rebase @prolic?

Contributor

gquemener commented Sep 5, 2018

Hello,

I've had some time to review the state of this PR.

Apart from the short message syntax and documentation, everything else has been implemented (which, I believe, already makes the feature usable).
My suggestion, mainly because I don't have time to take care on the syntax part, would be to focus on the documentation and merge it.

However, @prolic has added a default value feature which impacts this PR.
As a matter of fact, I've extracted the data constructor arguments parsing logic in a reusable function, in order to reuse it for the exception constructor arguments parsing.
The rebasing is not trivial and makes me think about conflict around the exception's ctor $message argument which also has a default value (defined through the _ => 'foo' syntax AFAIR).

Would you mind handling/trying this rebase @prolic?

@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Sep 5, 2018

Owner
Owner

prolic commented Sep 5, 2018

@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Sep 7, 2018

Owner

superseeded by #85

Owner

prolic commented Sep 7, 2018

superseeded by #85

@prolic prolic closed this Sep 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment