Skip to content

Explicitly document sort stability #158

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

Closed

Conversation

ford-prefect
Copy link

As not all users of this library might be aware of the affact that
Array.prototype.sort() can be stable or unstable depending on the actual
JavaScript runtime being used (and potentially the length of the array),
let's explicitly document this to avoid people from getting tripped up.

As not all users of this library might be aware of the affact that
Array.prototype.sort() can be stable or unstable depending on the actual
JavaScript runtime being used (and potentially the length of the array),
let's explicitly document this to avoid people from getting tripped up.
Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

Looks good. We'll need to remember to remove these after we do #155.

@sharno
Copy link
Contributor

sharno commented Oct 6, 2020

I think this is unnecessary now given this? :

tc39/ecma262#1340

@hdgarrood
Copy link
Contributor

Thanks for the link! I think we should document sort’s stability either way (whether it’s stable or not). #155 is likely going to be in the next release anyway though.

@JordanMartinez
Copy link
Contributor

Does #155 still need to be implemented? Or will merging this suffice due to the changes made to sort stability since this was made?

@thomashoneyman
Copy link
Member

With #195 it's unclear to me whether this is still relevant. Should this be closed?

@ford-prefect
Copy link
Author

ford-prefect commented Dec 23, 2020

If the new sortByImpl is a stable sort, then we can close this \o/

@hdgarrood
Copy link
Contributor

This should now be closed as the stability of sort no longer depends on your JS runtime. However, we should still document the stability of our sort, whether it is stable or not. I’m not sure if the new sort algorithm is stable - we should verify this, and if it is, we should document this and add tests for stability (so that we don’t accidentally lose stability later).

@JordanMartinez
Copy link
Contributor

I opened #196 to track the sort stability stuff.

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