-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
- Start Date: 2016-04-03 | ||
- RFC PR: | ||
- Ember Issue: | ||
|
||
# Summary | ||
|
||
Introduce a `uniqBy` function in the Enumerable mixin. | ||
|
||
` collection.uniqBy('name')` | ||
|
||
# Motivation | ||
|
||
The Enumerable mixin already provides `filterBy`, `sortBy`, `mapBy`, `rejectBy`, and `findBy`. It seems counterintuitive that the user has to write out the logic that would implement a `uniqBy` when the existing api conventions imply that it at the very least *could* exist. | ||
|
||
Also, the `uniq` function that does exist is limited to only working with primitive data types. A `uniqBy` would extend application of the method to cover objects as well. | ||
|
||
# Detailed design | ||
|
||
```js | ||
*/ | ||
@method uniqBy | ||
@param {String} property name(s) to uniqify on | ||
@return {Ember.Enumerable} | ||
@public | ||
*/ | ||
uniqBy() { | ||
var uniqKeys = arguments; | ||
var ret = emberA([]); | ||
|
||
this.forEach((item) => { | ||
var options = ret; | ||
|
||
for (var i = 0; i < uniqKeys.length; i++) { | ||
var key = uniqKeys[i]; | ||
|
||
options = options.filter(iter.apply(options, [key, item[key]])); | ||
} | ||
|
||
if (options.length <= 0) { | ||
ret.push(item); | ||
} | ||
}); | ||
|
||
return ret; | ||
}, | ||
``` | ||
|
||
Or something similar. | ||
# How We Teach This | ||
|
||
We would need additional documentation on the Ember.Enumerable class documentation, but otherwise it should not require any additional efforts to teach. | ||
|
||
# Drawbacks | ||
|
||
None that I'm aware of - perhaps poor performance implications when called on enumerables of great length? But this would be a drawback of the implementation, and not so much a commentary on the idea itself. | ||
|
||
# Alternatives | ||
|
||
The alternative is to continue having the users of Ember write their own functions to return unique values. Which, while not terrible, would be less convenient than if Ember already provided it. | ||
|
||
# Unresolved questions | ||
|
||
None that I know of. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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)
?There was a problem hiding this comment.
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.