-
-
Notifications
You must be signed in to change notification settings - Fork 921
Allow abstract resource #311
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
Conversation
5c87d16
to
e6d04dc
Compare
👍 |
👍 Can you add some tests? |
I'm on it. I wonder if we should remove POST and PUT routes to the abstract resource (because can't instanciate abstract class when denormalize the input data) |
0edf6a7
to
5c17ca9
Compare
Yes looks good to remove such methods as they can be added manually if necessary. |
@dunglas Should be OK now, but travis wont build my PR :( |
Maybe because of the conflict. Can you rebase your PR? |
9479cfa
to
d2eb55c
Compare
Rebased. With an assert equals of > 1000 lines, the doc.feature is no more a test nor maintenable... Shouldn't we removing it? |
c2da7ba
to
49b4363
Compare
Many tests needs to be refactored. Removing it totally is not an option but IMO we should test only some keys in the doc (the description of some selected endpoints) to ease the refactoring. |
@jderusse related to the tests, I think the way it has been done in #242 is good. |
LGTM, thank you @jderusse ! 👍 |
*/ | ||
private function createOperations(ContainerBuilder $container, $serviceId, $methods, $collection) | ||
{ | ||
return array_map(function ($method) use ($container, $serviceId, $collection) { |
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.
Why array_map
and not a simple foreach
?
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.
Question if habit, avoid temporary variable and gain 3 lines of code.
I can revert it if you prefer something more explicit
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.
Using methods as key should make the call more explicit and more performant (even if it not so important here because this method should not be called at runtime).
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 prefer the array_map
version too
Tests like in #242 is the way to go! |
👍 great. |
This PR needs yet another rebase! :) |
e70f1c6
to
3d92bab
Compare
I rebase the PR. |
No I just need to find the time to merge it in master too. If you're in a hurry can you open the PR against master yourself and I'll merge both. |
3d92bab
to
4caaf3e
Compare
4caaf3e
to
e2ed79d
Compare
here we go :) #328 |
Thank you @jderusse |
Given the following entities
When I configure configure ApiBundle to provider getters on
/notifications
(the abstract class)I should retrieves a list of MailNotification and PushNotification but got an exception
No resource found for object of type "MailNotification"
This PR allow: