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

util: Fix number format for pad #21906

Closed
wants to merge 1 commit into from
Closed

util: Fix number format for pad #21906

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 20, 2018

pad is now using toString(10), actually we don't need to do this. As for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString, toString(N) is a radix converter, which isn't proper here for time conversion.

PS:Since this is a private function with n a decimal number, so I suspect the code writer wanted to use toString(10) to convert a number to a two-digit number, and if the number < 10, a 0 is automatically added before the number itself. Obveriously this isn't a right way. Though the final answer is right.

Hope you all take what I mean.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

lib/util.js Outdated
@@ -1234,7 +1234,7 @@ function isPrimitive(arg) {
}

function pad(n) {
return n < 10 ? `0${n.toString(10)}` : n.toString(10);
return n < 10 ? `0${n}` : n.toString();
Copy link
Member

Choose a reason for hiding this comment

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

At this point, we can likely just use padString() ... e.g.

function pad(n) {
  return String(n).padStart(2, 0);
}

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it, sorry to forget that!
Many thanks!

@lpinca
Copy link
Member

lpinca commented Jul 22, 2018

@ghost
Copy link
Author

ghost commented Jul 31, 2018

@lpinca:It seems that the CI is passed but the status is still pending (in yellow). So I rebase and merged from the latest original Node and submit again. Is it need to restart another CI ? If yes, plz help me.
Thanks anyway!

@lpinca
Copy link
Member

lpinca commented Jul 31, 2018

@ghost ghost mentioned this pull request Aug 4, 2018
4 tasks
`pad` is now using `toString(10)`, actually we don't need to do this. As for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString, `toString(N)` is a radix converter, which isn't proper here for time conversion.
@ghost
Copy link
Author

ghost commented Aug 11, 2018

@Trott:Maybe this can merged?

@jasnell
Copy link
Member

jasnell commented Aug 11, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
@Trott
Copy link
Member

Trott commented Aug 11, 2018

@Trott Trott added the util Issues and PRs related to the built-in util module. label Aug 11, 2018
@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

Try freebsd ci again: https://ci.nodejs.org/job/node-test-commit-freebsd/19571/

@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

Seemingly unrelated failure in freebsd... trying once again https://ci.nodejs.org/job/node-test-commit-freebsd/19575/

@BridgeAR
Copy link
Member

BridgeAR commented Aug 13, 2018

@ghost
Copy link
Author

ghost commented Aug 21, 2018

@BridgeAR:Can this be merged? It seems test-commit always with errors... :(
The state is pending.....

@rubys
Copy link
Member

rubys commented Aug 21, 2018

Rebuilding freebsd again: https://ci.nodejs.org/job/node-test-commit-freebsd/19869/; failure matches #22317 which indicates that #22301 temporarily fixes the problem.

@ghost
Copy link
Author

ghost commented Aug 21, 2018

@Trott:Free-bsd passes correctly.

@rubys
Copy link
Member

rubys commented Aug 21, 2018

Landed: 6e9e150

Thanks!

@rubys rubys closed this Aug 21, 2018
rubys pushed a commit that referenced this pull request Aug 21, 2018
`pad` is now using `toString(10)`, actually we don't need to do this. As for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString, `toString(N)` is a radix converter, which isn't proper here for time conversion.

PR-URL: #21906
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
@ghost
Copy link
Author

ghost commented Aug 21, 2018

Thanks all :D

targos pushed a commit that referenced this pull request Aug 21, 2018
`pad` is now using `toString(10)`, actually we don't need to do this. As for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString, `toString(N)` is a radix converter, which isn't proper here for time conversion.

PR-URL: #21906
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
targos pushed a commit that referenced this pull request Sep 3, 2018
`pad` is now using `toString(10)`, actually we don't need to do this. As for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString, `toString(N)` is a radix converter, which isn't proper here for time conversion.

PR-URL: #21906
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Ruby <rubys@intertwingly.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants