Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
`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>
- Loading branch information
6e9e150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rubys ... quick nit... commit messages should be line wrapped at <= 80 chars
That's something that the individual landing needs to verify before pushing.
6e9e150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
git node land
via node-core-utils would have flagged it. One gotcha, though, is that if it kicks you out to the shell to rebase or amend a commit message or whatever, you have to remember to typegit node land --continue
to resume the process and get the check.If you're interested in running the check from the command line
npx core-validate-commit
will lint the most recent commit message.6e9e150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe another thing this reveals is we ought to take more time to check commit messages for correct formatting during the PR review process and (especially for frequent committers) block on PRs with commits like this one that aren't correctly formatted.
6e9e150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do better in the future. Yes,
git node land
did kick me out to the shell, and did provide a message that I didn't fully understand at the time (but now in retrospect makes perfect sense), but I haven't been through that process enough to realize that being kicked out to the shell isn't normal.6e9e150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nodejs/node-core-utils#160 in case anyone is interested ;)
6e9e150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spirit of tools-not-rules, and having CI catch stuff so committers don't have to: #22452
6e9e150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be lowercase as well.