Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: ambiguity in pull-requests.md #28068

Closed
rexagod opened this issue Jun 5, 2019 · 7 comments
Closed

doc: ambiguity in pull-requests.md #28068

rexagod opened this issue Jun 5, 2019 · 7 comments

Comments

@rexagod
Copy link
Member

rexagod commented Jun 5, 2019

Hi! I was going through the pull-requests.md file recently and noticed something that seemed a bit confusing to me, which is, in the 4th step the sample commit message says this:

Body of commit message is a few lines of text, explaining things
in more detail, possibly giving some background about the issue
being fixed, etc.

The body of the commit message can be several paragraphs, and
please do proper word-wrap and keep columns shorter than about
72 characters or so. That way, `git log` will show things
nicely even when it is indented.

Here the term body of commit message seems to be indicating two different instructions, was this intentional? Shouldn't the second one say something like the body of commit description instead (since only the first line should be preferably 0-50 chars while the description needs to wrapped around 72 columns, as indicated in the second paragraph above)? If not I guess it would make sense to combine both of the above statements into one if they seem to draw facts only about the body of commit message.

Also, since I'm a new contributor at the moment, I'm taking this up, in case this issue gets approved for a PR. Thanks!

@Trott
Copy link
Member

Trott commented Jun 6, 2019

Also, since I'm a new contributor at the moment, I'm taking this up, in case this issue gets approved for a PR. Thanks!

There is no "issue gets approved for a PR". You can open a PR any time you have a change you believe will improve the code base/docs.

@Trott
Copy link
Member

Trott commented Jun 6, 2019

To me, the commit message is the whole thing. The body of the commit message is all of the text except the first line and the trailing metadata (which the contributor doesn't provide anyway). I think "body" and "description" would refer to the same thing and would want us to stick to one term or the other, rather than use two terms for the same thing.

That said, if others don't find that intuitive, then let's do something else. The text is for new contributors, so whatever is clear to them.

@sam-github
Copy link
Contributor

I had to stare at this a lot, but I think what the OP is pointing out is that

Body of commit message is a few lines of text,

and

The body of the commit message can be several paragraphs

seem to be saying contradictory things, is it a few lines, or several paragraphs? They might have reached the conclusion that the two sections in our docs refer to different things.

@Trott
Copy link
Member

Trott commented Jun 6, 2019

I had to stare at this a lot, but I think what the OP is pointing out is that

Body of commit message is a few lines of text,

and

The body of the commit message can be several paragraphs

seem to be saying contradictory things, is it a few lines, or several paragraphs? They might have reached the conclusion that the two sections in our docs refer to different things.

Ah! That can stand some clarification for sure.

@rexagod
Copy link
Member Author

rexagod commented Jun 6, 2019

@sam-github, exactly and I can confirm that as a newer contributor this particular snippet in the aforementioned file was very confusing to me, hence my suggestions to edit this.

(...) want us to stick to one term or the other, rather than use two terms for the same thing.

@Trott, my thoughts exactly, and therefore I suggested to combine both of these paragraphs under a single term (i.e., body of the commit message) so that it doesn't sound ambiguous to newer contributors in the future.

Can I go ahead with this?

@rexagod
Copy link
Member Author

rexagod commented Jun 6, 2019

Ah! That can stand some clarification for sure.

Okay I saw this just now, so @Trott @sam-github, submitting a PR soon!

Trott pushed a commit to rexagod/node that referenced this issue Jun 9, 2019
Resolved ambiguous meanings for the term `body of commit message`
in `pull-requests.md` file,

Fixes: nodejs#28068
@rowlandjumbo
Copy link

Hi, why I think it is ok as it is is because some persons might not know what a paragraph looks like so the second sentence started on a new paragraph that showed exactly what a paragraph looks like.

@Trott Trott closed this as completed in 558f834 Jun 16, 2019
BridgeAR pushed a commit that referenced this issue Jun 17, 2019
Resolved ambiguous meanings for the term `body of commit message`
in `pull-requests.md` file,

Fixes: #28068
PR-URL: #28125
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Jun 18, 2019
Resolved ambiguous meanings for the term `body of commit message`
in `pull-requests.md` file,

Fixes: #28068
PR-URL: #28125
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants