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

RFC doc for adding uniqBy to enumerable #130

Closed

Conversation

lorrocha
Copy link

@lorrocha lorrocha commented Apr 3, 2016

No description provided.

@rwjblue
Copy link
Member

rwjblue commented Apr 3, 2016

Was this proposed to support the efforts in emberjs/ember.js#12875?

@lorrocha
Copy link
Author

lorrocha commented Apr 4, 2016

Ah, it is now! I snooped around for an RFC for it and didn't find any either pending or accepted, so I thought I would submit my own. I didn't think to look at the open PRs as I thought, as an api change, it would require an RFC before implementation. Sorry about that!

@rwjblue
Copy link
Member

rwjblue commented Apr 4, 2016

No apologies, you are absolutely right! I think this RFC applies to that PR very well.

@lorrocha
Copy link
Author

lorrocha commented Apr 4, 2016

I'll update the rfc to remove the specifics to the enumerable mixin!

for (var i = 0; i < uniqKeys.length; i++) {
var key = uniqKeys[i];

options = options.filter(iter.apply(options, [key, item[key]]));
Copy link
Member

Choose a reason for hiding this comment

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

is this supposed to be get(item, key)?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is! I'll update it in a bit.

@stefanpenner
Copy link
Member

This seems reasonable, we should discuss at the next meeting.

@stefanpenner
Copy link
Member

This is already in ember, we should be good.

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