util: Fix number format for pad#21906
Uh oh!
There was an error while loading. Please reload this page.
Conversation
lib/util.js Outdated
There was a problem hiding this comment.
At this point, we can likely just use padString() ... e.g.
functionpad(n){returnString(n).padStart(2,0);}There was a problem hiding this comment.
Fixed it, sorry to forget that!
Many thanks!
lpinca commented Jul 22, 2018
ghost commented Jul 31, 2018
@lpinca：It seems that the CI is passed but the status is still |
lpinca commented Jul 31, 2018
ghost mentioned this pull request `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 commented Aug 11, 2018
@Trott：Maybe this can merged? |
jasnell commented Aug 11, 2018
Trott added the author ready 
Trott commented Aug 11, 2018
Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16362/ |
jasnell commented Aug 12, 2018
Try freebsd ci again: https://ci.nodejs.org/job/node-test-commit-freebsd/19571/ |
jasnell commented Aug 12, 2018
Seemingly unrelated failure in freebsd... trying once again https://ci.nodejs.org/job/node-test-commit-freebsd/19575/ |
Rebuilding only the failed freebsd: |
ghost commented Aug 21, 2018
@BridgeAR：Can this be merged? It seems test-commit always with errors... :( |
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 commented Aug 21, 2018
@Trott：Free-bsd passes correctly. |
rubys commented Aug 21, 2018
Landed: 6e9e150 Thanks! |
`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 <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Ruby <[email protected]>
ghost commented Aug 21, 2018
Thanks all :D |
`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 <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Ruby <[email protected]>
`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 <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Ruby <[email protected]>
7 participants
padis now usingtoString(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
na decimal number, so I suspect the code writer wanted to usetoString(10)to convert a number to a two-digit number, and if the number < 10, a0is 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), orvcbuild test(Windows) passes