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

replace deprecated contains with includes #456

Merged

Conversation

GavinJoyce
Copy link
Collaborator

@GavinJoyce GavinJoyce commented Feb 20, 2017

  • Upgrade to current LTS (2.8.3)
  • Replace deprecation contains with includes

I'll address the getOwner deprecations once this has merged

@TRMW
Copy link

TRMW commented May 1, 2017

Would love to see this merged!

@GavinJoyce
Copy link
Collaborator Author

GavinJoyce commented May 2, 2017

@ebryn might it be possible to either merge this and other valid PRs or perhaps give commit rights to someone else who can? Thanks

@@ -335,7 +335,7 @@ test("should be able to set nonembedded relationship to null", function() {
});

equal(post.get('author'), null);
deepEqual(post.toJSON(), {id: 1, author_id: null});
deepEqual(post.toJSON(), {id: 1, author_id: undefined});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eventualbuddha I'm not sure what change in Ember resulted in this change. I'll track it down

Copy link
Collaborator Author

@GavinJoyce GavinJoyce Jul 12, 2017

Choose a reason for hiding this comment

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

There seems to have been a change within Ember between 2.0 and 2.8 which looks like a bug fix:

2.0:

screen shot 2017-07-12 at 22 01 01

2.8:

screen shot 2017-07-12 at 22 02 18

@GavinJoyce
Copy link
Collaborator Author

@eventualbuddha perhaps you might be able to review this? LMK if you can't and I'll try find someone else

@eventualbuddha
Copy link
Collaborator

Ping @GavinJoyce

@GavinJoyce
Copy link
Collaborator Author

@eventualbuddha I've added some context to the reason that the test change was needed above. Good to merge?

@eventualbuddha eventualbuddha merged commit f57a006 into ebryn:master Jul 24, 2017
@eventualbuddha
Copy link
Collaborator

Thanks @GavinJoyce!

@GavinJoyce GavinJoyce deleted the gj/replace-deprecated-contains branch February 22, 2018 11:28
@GavinJoyce GavinJoyce mentioned this pull request Apr 16, 2018
3 tasks
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.

None yet

3 participants