doc: improve documentation for commit subject line#8546
Conversation
6ed8a91 to
9491458
Compare
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
maybe present-tense or imperative verb? Present tense is easier to understand, but imperative is I think more accurate.
There was a problem hiding this comment.
@gibfahn I used present-tense here as per this comment: #8479 (comment).
Not sure but there seems to be a risk of bike-shedding.
The risk is confirmed. I'm happy to use whatever is best for the purpose.
There was a problem hiding this comment.
To be clear, I don't mind either, I just thought it might be easiest to have both, thus avoiding the issue.
There was a problem hiding this comment.
That's fine with me, updating.
9491458 to
0fc4e81
Compare
Fishrock123
left a comment
There was a problem hiding this comment.
I would change must to should -- we don't need more hard barriers for PRs, and we can always fix these up ourselves.
|
@Fishrock123 both of them? |
jasnell
left a comment
There was a problem hiding this comment.
LGTM with the changes! Thank you!
Trott
left a comment
There was a problem hiding this comment.
Left a nit, but this LGTM with or without the nit addressed.
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
Nit: The exception is not quite broad enough. We also make exceptions for proper nouns (Linux Foundation), acronyms (MIT license), and probably other things I'm forgetting. How to communicate this succinctly is a challenge, though, and this LGTM as-is.
There was a problem hiding this comment.
Added a commit to specify that proper nouns and acronyms should not be lowercased.
If we are forgetting something I think we can always add it later.
|
LGTM |
|
@Fishrock123 are you ok with the changes? |
|
Ping @Fishrock123 |
CONTRIBUTING.md
Outdated
There was a problem hiding this comment.
I'm fine with this landing as-is. I want to add: I know I'm the person who said present-tense may be preferable to imperative verb. I'm starting to have second thoughts. fixing would be present tense but is not OK. An imperative verb will always be OK (I think). So if you'd prefer to just use imperative and drop present-tense, I'd be OK with that.
So, yeah, either way, LGTM.
Nit: I prefer we write out for example rather than rely on e.g.. Again, LGTM as is, and LGTM with that change. Either way.
There was a problem hiding this comment.
@Trott I'll address those two nits tomorrow, I'm too sleepy now. Just let me know what's better to use. @gibfahn suggested "present-tense or imperative" to avoid confusion but I also think that it is not totally correct and only "imperative" would be better. As a non native speaker I can't make a decision.
There was a problem hiding this comment.
I now favor just "imperative verb" and dropping "present-tense" but if anyone (@gibfahn or otherwise) feels that including present-tense is better, that's OK too.
|
PTAL. |
|
I agree with that Imperative is more accurate, I assumed the idea behind present tense was to make things easier for non-native speakers, but as @Trott mentioned, it would probably add to their confusion rather than making things clearer. Therefore the changes LGTM, thanks for dealing with all the nits @lpinca |
|
LGTM |
|
@Fishrock123 ping. |
|
@Fishrock123 This looks like it's stalled on your review, which seems like a nit more than anything else. This can land, yes? |
|
go ahead if you wana land it :D |
Specify that commit subject line must be made of only lowercase words and should start with an imperative verb. PR-URL: nodejs#8546 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
93f5e9f to
a4d396d
Compare
|
Landed in a4d396d |
Specify that commit subject line must be made of only lowercase words and should start with an imperative verb. PR-URL: #8546 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Specify that commit subject line must be made of only lowercase words and should start with an imperative verb. PR-URL: #8546 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Checklist
Affected core subsystem(s)
doc
Description of change
Specify that commit subject line must be made of only lowercase words and should start with an imperative verb.