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

[BUGFIX beta] Rename relationship.destroy to removeInverseRelationships #4898

Merged
merged 1 commit into from
Mar 28, 2017

Conversation

sduquej
Copy link
Contributor

@sduquej sduquej commented Mar 27, 2017

Fixes #4752

The semantics of destroy have changed since 2f18405. This PR renames the method to convey better what it does.

@sduquej sduquej changed the title [BUGFIX beta] Rename relationship.relation to removeInverseRelationships [BUGFIX beta] Rename relationship.destroy to removeInverseRelationships Mar 27, 2017
@sduquej sduquej changed the title [BUGFIX beta] Rename relationship.destroy to removeInverseRelationships [BUGFIX beta] Rename relationship.destroy to removeInverseRelationships Mar 27, 2017
@bmac
Copy link
Member

bmac commented Mar 28, 2017

Looks good @sduquej. Would you mind rebasing this pr? I just want to make sure it passes CI since its renaming a method.

@sduquej sduquej force-pushed the rename-relationship-destroy branch from 97895b3 to a9f91de Compare March 28, 2017 16:58
@sduquej
Copy link
Contributor Author

sduquej commented Mar 28, 2017

Done! 🤞

@bmac bmac merged commit b7166f6 into emberjs:master Mar 28, 2017
@bmac
Copy link
Member

bmac commented Mar 28, 2017

Thanks @sduquej.

@sduquej sduquej deleted the rename-relationship-destroy branch March 28, 2017 19:36
@runspired
Copy link
Contributor

We should revert this PR, destroy() was the correct method name as it was invoked when tearing down the instance, and it is not expected that you can use a relationship after it is called.

@runspired
Copy link
Contributor

Furthermore, this was definitely not a bugfix, or needed for beta.

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.

3 participants