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.

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

Bring back relationship parameter for adapter findHasMany #2252

Closed
wants to merge 2 commits into from
Closed

Bring back relationship parameter for adapter findHasMany #2252

wants to merge 2 commits into from

Conversation

intuitivepixel
Copy link

Signed-off-by: Alessandro D'Aquino (elara) [email protected]

@@ -99,7 +99,7 @@ test("A serializer can materialize a hasMany as an opaque token that can be lazi
throw new Error("Adapter's findMany should not be called");
};

env.adapter.findHasMany = function(store, record, link) {
env.adapter.findHasMany = function(store, record, link, relationship) {
equal(link, "/posts/1/comments", "findHasMany link was /posts/1/comments");
Copy link
Member

Choose a reason for hiding this comment

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

Wanna assert that the relationship is passed in here correctly?

Copy link
Author

Choose a reason for hiding this comment

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

@igorT looks better now?

Copy link
Author

Choose a reason for hiding this comment

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

@igorT should I squash the two commits?

Copy link
Member

Choose a reason for hiding this comment

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

Squashing would be nice 👍

Copy link
Author

Choose a reason for hiding this comment

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

@igorT I'm going to open a new PR with the squashed commits, will post here the new PR

@intuitivepixel
Copy link
Author

@igorT I hope you are still happy to merge this PR, I really need this so I can get rid of my patched version of ED, thanks.

@igorT
Copy link
Member

igorT commented Sep 8, 2014

Looks good so far

@@ -431,7 +431,7 @@ export default Adapter.extend({
@param {String} url
@return {Promise} promise
*/
findBelongsTo: function(store, record, url) {
findBelongsTo: function(store, record, url, relationship) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like you changed the type into a relationship in the HasMany, but not in the belongsTo? Also test?

Copy link
Author

Choose a reason for hiding this comment

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

@igorT I didn't need to change it because the relationship parameter is already passed in: https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/store.js#L1788, and the tests already check for it: https://github.com/emberjs/data/blob/master/packages/ember-data/tests/integration/relationships/belongs_to_test.js#L178 or am I missing something here?

@intuitivepixel
Copy link
Author

@igorT closing this PR in favour of a new one with squashed commits: #2263. Regarding findBelongsTo I didn't change that part because as stated above it's already half covered including tests, it was just missing the parameter in the rest-adapter findBelongsTo method signature. Please let me know if this is finally 👍 to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants