Uh oh!
There was an error while loading. Please reload this page.
doc: Stream 'finish' event string interpolation fix - #12221
Conversation
Good catch and thanks for the contribution! Can you please format your commit message according to the commit guidelines? The first word after the subsystem must start with a lowercase letter (and should be an imperative verb) and the whole line must not exceed 50 characters. |
vhmth
commented
Apr 4, 2017
Done! Should fit in 50 chars now too |
aqrln
commented
Apr 4, 2017
@vhmth great! @vsemozhetbyt IMO, the change is trivial enough to not wait for 48 hours to land. What do you think? |
@aqrln I am still a bit uncertain about timeframe in these cases. @nodejs/documentation, how quick can such fixes be landed? |
jasnell
commented
Apr 4, 2017
The minimum time for most PRs is 48 hours, 72 hours over the weekend. We can shorten the time for very trivial doc changes but those are rare. |
vhmth
commented
Apr 4, 2017
Don't worry guys we have all the time in the world once singularity hits. 🤖 |
vsemozhetbyt
commented
Apr 4, 2017
@jasnell Is this trivial? If so, how much can the time be shorten? |
Trott
commented
Apr 4, 2017
There's no hard-and-fast rules on what is "trivial" and how much things can be shortened. Use your judgment. (And if your judgment is way off, someone will tell you. The CTC can always revise the rules if there's an epidemic of PRs landing too quickly.) In my opinion, this is definitely trivial enough that you can land it right now. (I generally try to get at least two other people to say "yeah, land this right away", if that helps.) It's a tiny doc change that is easy and harmless to undo if it turns out there's something wrong with it that we're all missing. |
Trott
commented
Apr 4, 2017
(All that said: If in doubt, just wait the 48 hours. There's no huge benefit to trivial docs landing faster other than it keeping them out of our backlog and not frustrating contributors who just want to add a missing apostrophe in a doc or something like that.) |
vhmth
commented
Apr 4, 2017
Are y'all able to set up a system where PRs get auto-merged at the 48hr mark with a certain amount of approvals? |
jasnell
commented
Apr 4, 2017
@vhmth ... auto-merging generally is not a great approach. In addition to the 48 hours, most PRs require one or more CI runs, may need additional testing using our "Canary in the Gold Mine" (CITGM) tool, or otherwise just need another set of eyes to sanity check things. There are also a number of requirements we have for squashing and formatting commit messages that we could automate but there's little apparent benefit in doing so. |
jasnell
commented
Apr 4, 2017
btw, +1 to this being trivial enough to land whenever you're ready. |
PR-URL: #12221 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
vsemozhetbyt
commented
Apr 4, 2017
vhmth
commented
Apr 4, 2017
Boom! |
PR-URL: nodejs#12221 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #12221 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #12221 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: nodejs/node#12221 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Checklist
Affected core subsystem(s)
doc
Description of change
String interpolation in the docs in the
Stream 'finish'event example does not use back ticks `.