| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM with the mentioned nits concerning commit message.
Sorry, something went wrong.
|
Done! Should fit in 50 chars now too |
Sorry, something went wrong.
|
@vhmth great! @vsemozhetbyt IMO, the change is trivial enough to not wait for 48 hours to land. What do you think? |
Sorry, something went wrong.
|
@aqrln I am still a bit uncertain about timeframe in these cases. @nodejs/documentation, how quick can such fixes be landed? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Don't worry guys we have all the time in the world once singularity hits. 🤖 |
Sorry, something went wrong.
|
@jasnell Is this trivial? If so, how much can the time be shorten? |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
(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.) |
Sorry, something went wrong.
|
Are y'all able to set up a system where PRs get auto-merged at the 48hr mark with a certain amount of approvals? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
btw, +1 to this being trivial enough to land whenever you're ready. |
Sorry, something went wrong.
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>
Sorry, something went wrong.
|
Boom! |
Sorry, something went wrong.
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>
| Back | FazBrowse Home | New Git URL |
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 `.