[BUGFIX] (Bug #2714) For _.uniq which acted incorrectly on sorted lists using a non injective iteratee #2715
Conversation
This file contains hidden or 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
…d list with a non injective iteratee and added a test case to test this fix
|
This is my first pull request ever, so please, kindly point out if I am doing something wrong, or if I have made any mistakes. Thank you so much. |
jashkenas
requested changes
Nov 27, 2017
underscore.js
Outdated
| @@ -558,6 +558,8 @@ | |||
|
|
|||
| // Produce a duplicate-free version of the array. If the array has already | |||
| // been sorted, you have the option of using a faster algorithm. | |||
| // The faster algorithm will not work with an iteratee if the iteratee is not a one-to-one function, so providing an iteratee will disable the faster algorithm | |||
There was a problem hiding this comment.
Comments should be complete sentences, with punctuation, wrapped to an ~80 line width.
underscore.js
Outdated
| @@ -558,6 +558,8 @@ | |||
|
|
|||
| // Produce a duplicate-free version of the array. If the array has already | |||
| // been sorted, you have the option of using a faster algorithm. | |||
| // The faster algorithm will not work with an iteratee if the iteratee is not a one-to-one function, so providing an iteratee will disable the faster algorithm | |||
| // Perhaps a warning should be thrown if an iteratee is provided while isSorted is true, to warn that the faster algorithm will not be used | |||
There was a problem hiding this comment.
Let's get rid of this sentence — we don't need to publish it along with the library.
|
Thanks! I updated the lines that the bot flagged, and now I will update the code with the comments that you added. In a minute I will make another commit. |
|
I fixed the comments in the code as per your request. Let me know if you would like me to change anything else. |
3 similar comments
jashkenas
approved these changes
Nov 27, 2017
|
Nice work! |
|
Thank you, I really appreciate it! |
mAiNiNfEcTiOn
pushed a commit
to mAiNiNfEcTiOn/interview-process
that referenced
this pull request
Mar 2, 2018
[BUGFIX] (Bug jashkenas#2714) For _.uniq which acted incorrectly on sorted lists using a non injective iteratee
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When using an iteratee that is not a one-to-one function, and isSorted=true, then _.uniq would attempt to use the fast algorithm which fails for non one-to-one (injective) functions, because they map to resultant values which are not sorted.