doc: improve documentation for commit subject line #8546
Conversation
lpinca
force-pushed
the
update/commit-message-guidelines
branch
from
6ed8a91 to
9491458
Compare
| description of the change. All words in the description must be in lowercase | ||
| with the exception of the ones that refer to code (e.g. function/variable | ||
| names). The description must be prefixed with the name of the changed | ||
| subsystem and should start with a present-tense verb (e.g. "net: add |
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.
lpinca
force-pushed
the
update/commit-message-guidelines
branch
from
9491458 to
0fc4e81
Compare
|
@Fishrock123 both of them? |
| description of the change prefixed with the name of the changed | ||
| subsystem (e.g. "net: add localAddress and localPort to Socket"). | ||
| description of the change. All words in the description should be in | ||
| lowercase with the exception of the ones that refer to code (e.g. |
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 |
| (e.g. "net: add localAddress and localPort to Socket"). | ||
| function/variable names), proper nouns, and acronyms. The description should | ||
| be prefixed with the name of the changed subsystem and start with a | ||
| present-tense or imperative verb (e.g. "net: add localAddress and localPort |
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
lpinca
force-pushed
the
update/commit-message-guidelines
branch
from
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
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.