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

Document the use of operator < #2848

Merged
merged 3 commits into from
May 7, 2020

Conversation

jgonggrijp
Copy link
Collaborator

@jashkenas I inserted a new "Notes" section between the links and the changelog. For now it only contains the note on operator <. Maybe that note is too long. Maybe the references are too short.

Implements #2846
Closes #2042
Closes #1834
Closes #1768

@jashkenas
Copy link
Owner

I love it, but it is a bit much 😉

Instead of such an excellent treatise on < and NaN behavior, perhaps try trimming it down to a version that simply briefly mentions that non-comparable JS values have undefined sort order behavior, perhaps with a link to good external docs on the matter, and then move on quickly to a more practical discussion about how to fix your sorting problems: By defining a custom comparator that allows you to specify a sub-key, or compute a value, by which to sort.

@jgonggrijp
Copy link
Collaborator Author

I do get carried away sometimes. 😅

Better now?

@jashkenas
Copy link
Owner

Looks great. The one thing I might consider adding would be what I consider the most common case of difficulties sorting in JS: sorting an array of objects or class instances. Generally, there’s a single property on the object, or a method to call, to produce the desired value for the sort iterator — but that’s not obvious to beginners!

It also makes for a good example snippet.

@jgonggrijp
Copy link
Collaborator Author

I agree there is a value in a general "sorting in Underscore for beginners" type of note. However, when writing this note I specifically set out to address issues like #1768, which I suspect people still run into after they have already figured out the property shorthand thing. After all, a property that is missing in some objects in a collection is a common way in which people may end up comparing undefined. The documentation of _.sortBy et al. already mentions the option to use an iteratee, anyway.

I think a general introduction to sorting for beginners would require a different approach. I'm not opposed to changing the note in order to suit this purpose, but I want to concentrate on other things right now. Given your approval, I'll just merge this for now. We can revise it at any time.

I have however added a link to the _.iteratee section to give a bit more of a hint of the available options.

@jgonggrijp jgonggrijp merged commit 5d8ab5e into jashkenas:master May 7, 2020
@jgonggrijp jgonggrijp deleted the comparison-operator-doc branch May 7, 2020 21:56
jgonggrijp added a commit to jgonggrijp/underscore that referenced this pull request Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_.sortedIndex align with _.sortBy Comparator edge-case handling
2 participants