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.

Sign up for GitHub

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

Jump to bottom

doc,test: fix inspect's sorted compare function #22992

Closed
wants to merge 1 commit into from
Closed

doc,test: fix inspect's sorted compare function #22992

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Sep 21, 2018

In V8 7.0, the array sorting algorithm was changed to Timsort, which
is stable. A compare function returning only true or false
(converted to 0 and 1) cannot work properly.

@targos
doc,test: fix inspect's sorted compare function
2afcf10 
In V8 7.0, the array sorting algorithm was changed to Timsort, which
is stable. A compare function returning only `true` or `false`
(converted to 0 and 1) cannot work properly.
@targos targos requested a review from BridgeAR September 21, 2018 09:31
@nodejs-github-bot
Copy link
Collaborator

@targos build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/955/pipeline

@targos targos added util Issues and PRs related to the built-in util module. doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. fast-track PRs that do not need to wait for 48 hours to land. labels Sep 21, 2018
@targos
Copy link
Member Author

targos commented Sep 21, 2018

👍 to fast-track and unblock #22754

@targos
Copy link
Member Author

targos commented Sep 21, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17352/

@richardlau
Copy link
Member

How does this compare to #23000 which is modifying the same bit of code in the test?

@richardlau richardlau mentioned this pull request Sep 21, 2018
Closed
4 tasks
@richardlau richardlau removed the fast-track PRs that do not need to wait for 48 hours to land. label Sep 21, 2018
@richardlau
Copy link
Member

👍 to fast-track and unblock #22754

I've removed the fast-track label as there is another PR #23000 proposing a different change.

@kfarnung
Copy link
Contributor

Does localeCompare require Intl support? Given that I don't see any other usages of localeCompare in the test directory, maybe my fix is preferrable?

@kfarnung kfarnung added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 21, 2018
@kfarnung
Copy link
Contributor

I added fast-track back, I think this is the cleaner change.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 21, 2018
@targos
Copy link
Member Author

targos commented Sep 22, 2018

@kfarnung Thank you. localeCompare doesn't require Intl support. I chose to use it because it's short and easy to read.

@targos
Copy link
Member Author

targos commented Sep 22, 2018

Landed in e758d4a

@targos targos closed this Sep 22, 2018
@targos targos deleted the util-inspect-stable branch September 22, 2018 12:32
targos added a commit that referenced this pull request Sep 22, 2018
@targos
doc,test: fix inspect's sorted compare function
e758d4a 
In V8 7.0, the array sorting algorithm was changed to Timsort, which
is stable. A compare function returning only `true` or `false`
(converted to 0 and 1) cannot work properly.

PR-URL: #22992
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
@targos targos mentioned this pull request Sep 22, 2018
Closed
5 tasks
targos added a commit to targos/node that referenced this pull request Sep 23, 2018
@targos
doc,test: fix inspect's sorted compare function
2b3af24 
In V8 7.0, the array sorting algorithm was changed to Timsort, which
is stable. A compare function returning only `true` or `false`
(converted to 0 and 1) cannot work properly.

PR-URL: nodejs#22992
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
@targos targos mentioned this pull request Sep 23, 2018
Closed
targos added a commit that referenced this pull request Sep 24, 2018
@targos
doc,test: fix inspect's sorted compare function
b48dc0b 
In V8 7.0, the array sorting algorithm was changed to Timsort, which
is stable. A compare function returning only `true` or `false`
(converted to 0 and 1) cannot work properly.

Backport-PR-URL: #23039
PR-URL: #22992
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
@targos targos added backported-to-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v10.x labels Sep 24, 2018
@targos targos mentioned this pull request Oct 7, 2018
Merged
This was referenced Oct 10, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@kfarnung kfarnung kfarnung approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@targos @nodejs-github-bot @richardlau @kfarnung @jasnell @cjihrig @BridgeAR @addaleax