Skip to content
This repository has been archived by the owner on Mar 22, 2019. It is now read-only.

Add Enumerable.contains to deprecation guide #2600

Merged
merged 1 commit into from Jul 5, 2016

Conversation

bmeurant
Copy link
Contributor

@bmeurant bmeurant commented May 25, 2016

Deprecation guide update related to emberjs/ember.js#13553

rendered

##### until: 3.0.0
##### id: ember-runtime.enumerable-contains

The `Enumerable.contains` and `Array.contains` methods were deprecated in favor of `Enumerable.includes` and `Array.includes`
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can we represent theses as Enumerable#contains? I tend to treat Enumerable.contains as a method that is literally on that thing (class usually), and #contains as a method that exists on an instance of that thing (object, mixin applied to object).

@mixonic
Copy link
Sponsor Member

mixonic commented May 27, 2016

I think this should also mention the behavior change of addObject and without to use includes instead of contains. It seems a pretty innocent breaking change, but it should be called out.


Note that the second `startAt` parameter is only available for `Ember.Array` because `Ember.Enumerable` does not rely on index-ordered access.

`Enumerable#without` and `MutableEnumerable#addObject` use now internally `includes` instead of `contains`. This leads to some changes:
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

"some minor breaking changes" perhaps

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 1, 2016

@bmeurant this is 100% gtg with a small patch and a rebase. Sorry for the trouble, but you can land that I'd I will merge? :-D

@bmeurant
Copy link
Contributor Author

bmeurant commented Jul 3, 2016

@mixonic sorry for the delay. I made the change and rebased. Should be ok now.

@mixonic mixonic merged commit a99f6ed into emberjs:master Jul 5, 2016
@mixonic
Copy link
Sponsor Member

mixonic commented Jul 5, 2016

👏 thank you @bmeurant! :-D

arr.includes('b'); // true
arr.includes('d'); // false
arr.includes(NaN); // true
arr.contains(null); // true
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

this line and the one below should be includes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooooops ! Yes it should be includes, I missed it, sorry. I can update but this is already merged 😩

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mixonic let me know if you want me to make a new PR to fix this

Copy link
Contributor

Choose a reason for hiding this comment

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

@bmeurant went ahead and fixed it since the change is already deployed. Thanks for the original work :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@locks 👍 thx. You're welcome

locks added a commit that referenced this pull request Jul 19, 2016
@locks locks mentioned this pull request Jul 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants