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

serialize:true takes priority over the OneToMany check for relationships #3214

Merged
merged 1 commit into from
Jun 11, 2015
Merged

serialize:true takes priority over the OneToMany check for relationships #3214

merged 1 commit into from
Jun 11, 2015

Conversation

sly7-7
Copy link
Contributor

@sly7-7 sly7-7 commented Jun 5, 2015

Fixes #3189

Apologize in advance for my frenglished documentation
cc/ @igorT

@property _mustSerialize
@private
*/
_mustSerialize: false,
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense as a flag, it's a value that changes for each key, so needs to be a function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did'nt thought about that, definitely I will remove that.

@igorT
Copy link
Member

igorT commented Jun 6, 2015

@sly7-7 we should implement shouldSerializeRelationship and that can implement this logic + oneToMany check and then we call that instead of the check on 692 and the

@sly7-7
Copy link
Contributor Author

sly7-7 commented Jun 6, 2015

@igorT I think I understand, I don't know when you want this, but I won't be able to do before tomorrow evening (trip today, soccer tomorrow). If it's not to late I'm happy to take that.

@igorT
Copy link
Member

igorT commented Jun 6, 2015

No rush, enjoy the game

@sly7-7
Copy link
Contributor Author

sly7-7 commented Jun 9, 2015

@igorT Ping ?

@igorT
Copy link
Member

igorT commented Jun 9, 2015

Seems to fail on Travis due to jshint

@sly7-7
Copy link
Contributor Author

sly7-7 commented Jun 9, 2015

@igorT I'm confused, I can't reproduce the jshint error locally. I just rebase and force push... let's see whan happen

@sly7-7
Copy link
Contributor Author

sly7-7 commented Jun 9, 2015

@igorT It's ready if you don't have any other (welcome) advices :)

@igorT
Copy link
Member

igorT commented Jun 9, 2015

@sly7-7 I think we talked about moving the

+      if (this._shouldSerializeHasMany(key, relationshipType)) {

to be next to the canSerialize line right?

igorT added a commit that referenced this pull request Jun 11, 2015
serialize:true takes priority over the OneToMany check for relationships
@igorT igorT merged commit 43ff3a5 into emberjs:master Jun 11, 2015
@igorT
Copy link
Member

igorT commented Jun 11, 2015

Thanks!

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.

4 participants