-
-
Notifications
You must be signed in to change notification settings - Fork 82
feat(interface): php_impl_interface macro #621
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
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 21382858121Details
💛 - Coveralls |
e5c61bb to
7febcb0
Compare
f47b1a1 to
174c959
Compare
This comment was marked as outdated.
This comment was marked as outdated.
4cb1cc0 to
240b4fb
Compare
240b4fb to
ce32533
Compare
|
Please review/merge. |
3d0cdd2 to
1a82750
Compare
|
@ptondereau Rebased and green. This one next? |
ptondereau
left a comment
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.
Nice first draft.
But let me add some suggestions:
- Can we add a compile-time error if
#[php_impl_interface]is used on a non-trait impl? - Maybe throw a warning if trait methods aren't also in
#[php_impl] - We should add an example in the documentation showing cross-crate interface implementation
7cb7add to
d3c2b8c
Compare
6459b5c to
bcd9f87
Compare
✅
Now there's no need to define them separately in php_impl.
✅ |
ptondereau
left a comment
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.
I'll do a final review tomorrow but for now LGTM
ptondereau
left a comment
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.
One more things 😅
f9e0e55 to
f733620
Compare
All done, Mr Colombo, but CI is acting weird |
thank you! |
f733620 to
98e4300
Compare
98e4300 to
9d51536
Compare
|
@kakserpom Maybe you might be interested in this #653 |
How is it related? |
Implements #590 and #308