doc: improve documentation for commit subject line#8546
Uh oh!
There was an error while loading. Please reload this page.
Conversation
lpincaforce-pushed the update/commit-message-guidelines branch from 6ed8a91 to 9491458Compare
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.
lpincaforce-pushed the update/commit-message-guidelines branch from 9491458 to 0fc4e81Compare
Fishrock123 left a comment
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.
lpinca commented Sep 15, 2016
@Fishrock123 both of them? |
jasnell left a comment
jasnell left a comment There was a problem hiding this comment.
LGTM with the changes! Thank you!
Trott left a comment
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.
gibfahn commented Sep 16, 2016
LGTM |
lpinca commented Sep 19, 2016
@Fishrock123 are you ok with the changes? |
jasnell commented Sep 22, 2016
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.
lpinca commented Sep 23, 2016
PTAL. |
gibfahn commented Sep 23, 2016
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 |
Trott commented Sep 23, 2016
LGTM |
lpinca commented Sep 28, 2016
@Fishrock123 ping. |
Trott commented Sep 29, 2016
@Fishrock123 This looks like it's stalled on your review, which seems like a nit more than anything else. This can land, yes? |
Fishrock123 commented Sep 29, 2016
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]>
lpincaforce-pushed the update/commit-message-guidelines branch from 93f5e9f to a4d396dComparelpinca commented Sep 29, 2016
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]>
9 participants
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.