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

Added static Immutable.sortBy(array[, sorter]) function #211

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

LukeusMaximus
Copy link

Adds a static sortBy function to the Immutable object. It exposes the basic JS Array#sort functionality. It can take an optional sorting function as per Array#sort.

@LukeusMaximus LukeusMaximus mentioned this pull request Mar 27, 2017
@LukeusMaximus
Copy link
Author

I'm unsure how to address the build issues in Travis. Can someone assist?

@foray1010
Copy link

would prefer if no change after sort, return the same reference

@LukeusMaximus
Copy link
Author

What's going on with this Travis build? I'm still unable to proceed.

@seanf
Copy link

seanf commented Jul 6, 2018

Build log mentions zuul. Probably related to saucelabs: #249 (comment)


for (i = 0, length = array.length; i < length; i++) {
result.push(array[i]);
}
Copy link

Choose a reason for hiding this comment

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

Why not use asMutable() instead of making a mutable copy manually?

// Check if both are NaN. Type checks catch cases where isNaN returns true for strings.
if(!(typeof result[i] === "number" && isNaN(result[i]) && typeof array[i] === "number" && isNaN(array[i]))) {
rearranged = true;
}
Copy link

Choose a reason for hiding this comment

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

should break here

@seanstrom
Copy link

@pekeler would this this pull request be ready to merge if the remaining review comments were resolved?

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.

5 participants