doc: make commit guidelines easier to reference #11732

bf4 · 2017-03-07T16:05:33Z

Can now link to [Commit Guidelines](CONTRIBUTING.md#commit-guidelines) from pull requests
Breaks up commit requirements and recommendations

Refs: #11723 (comment)

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

None

bf4 · 2017-03-07T16:07:13Z

(first attempt to use 'hub' to use the template.. failed)

Trott

Thanks for taking the time to do this! I definitely have some concerns about some of the links, but I do like the added header and the re-organization of the info.

Trott · 2017-03-07T19:24:15Z

CONTRIBUTING.md

@@ -12,6 +12,9 @@ When opening new issues or commenting on existing issues on this repository
 please make sure discussions are related to concrete technical issues with the
 Node.js software.

+Go to https://github.com/nodejs/node/issues/new and click the 'New Issue' button


I'm definitely -1 on including the bare URL. I'm not sure any of this paragraph is needed at all. Do we have any reason to believe that people don't know how to open issues on GitHub and are looking for the information in the CONTRIBUTING.md? Most people only see the CONTRIBUTING.md when they are already opening a PR or issue because that's when GitHub shows a link to it.

In retrospect, I agree. :)

Trott · 2017-03-07T19:25:00Z

CONTRIBUTING.md

@@ -12,6 +12,9 @@ When opening new issues or commenting on existing issues on this repository
 please make sure discussions are related to concrete technical issues with the
 Node.js software.

+Go to https://github.com/nodejs/node/issues/new and click the 'New Issue' button
+and [fill out the form](.github/ISSUE_TEMPLATE.md).


This link is misleading. The text implies that you will be taken to a form. Instead, it will take you to a markdown file. I think this entire paragraph should be removed.

Trott · 2017-03-07T19:25:53Z

CONTRIBUTING.md

@@ -203,7 +208,7 @@ $ git push origin my-branch
 ```

 Go to https://github.com/yourusername/node and select your branch.
-Click the 'Pull Request' button and fill out the form.
+Click the 'Pull Request' button and [fill out the form](.github/PULL_REQUEST_TEMPLATE.md).


This link is misleading. The text implies that you will be taken to a form. Instead, it will take you to a markdown file. I think this entire paragraph should be removed.

Trott · 2017-03-07T19:26:06Z

lib/internal/process/next_tick.js

@@ -2,7 +2,7 @@

 // This value is used to prevent the nextTickQueue from becoming too
 // large and cause the process to run out of memory. When this value
-// is reached the nextTimeQueue array will be shortend (see tickDone
+// is reached the nextTimeQueue array will be shortened (see tickDone


Unrelated change.

@bf4 could you raise this in a separate PR.

Sure. Didn't know which would ultimately be more overhead

Trott · 2017-03-07T19:26:11Z

test/README.md

@@ -5,7 +5,7 @@ This folder contains code and data used to test the Node.js implementation.
 For a detailed guide on how to write tests in this
 directory, see [the guide on writing tests](../doc/guides/writing-tests.md).

-On how to run tests in this direcotry, see
+On how to run tests in this directory, see


Unrelated change.

Trott · 2017-03-07T19:26:58Z

CONTRIBUTING.md

-   lowercase with the exception of proper nouns, acronyms, and the ones that
-   refer to code, like function/variable names. The description should
-   be prefixed with the name of the changed subsystem and start with an
+#### Commit Message Guidelines


👍 to adding a header for the formatting guidelines.

Also 👍 to the re-organization and consolidation.

gibfahn

Some nits, but a big +1 on the new layout

gibfahn · 2017-03-07T21:32:20Z

CONTRIBUTING.md

+1. The first line (the header) should:
+   - contain a short description of the change.
+   - be 50 characters or less.
+   - be entirely in lowercase with the exception of proper nouns, acronyms, and the ones that


Maybe the ones -> words

gibfahn · 2017-03-07T21:33:01Z

CONTRIBUTING.md

+#### Commit Message Guidelines
+
+1. The first line (the header) should:
+   - contain a short description of the change.


I don't think we need full stops at the end of these, it's all one run-on sentence after the colon (I guess you could use commas if you wanted to, but I don't think it's helpful).

gibfahn · 2017-03-07T23:08:09Z

lib/internal/process/next_tick.js

@@ -2,7 +2,7 @@

 // This value is used to prevent the nextTickQueue from becoming too
 // large and cause the process to run out of memory. When this value
-// is reached the nextTimeQueue array will be shortend (see tickDone
+// is reached the nextTimeQueue array will be shortened (see tickDone


@bf4 could you raise this in a separate PR.

gibfahn · 2017-03-07T23:11:23Z

CONTRIBUTING.md

-   be prefixed with the name of the changed subsystem and start with an
+#### Commit Message Guidelines
+
+1. The first line (the header) should:


the header -> the subject line (I've never seen it called the header)

It's referred to as the header below. I was just conserving the language so that later references to 'the header' would be understandable.

Okay, but there don't seem to be any more references to "the header" as of this PR, so I think we should be fine using subject line instead?

gibfahn · 2017-03-07T23:12:44Z

CONTRIBUTING.md

+   - be 50 characters or less.
+   - be entirely in lowercase with the exception of proper nouns, acronyms, and the ones that
+   refer to code, like function/variable names.
+   - be prefixed with the name of the changed subsystem and start with an
   imperative verb. Example: "net: add localAddress and localPort


Example -> Examples

gibfahn · 2017-03-07T23:13:39Z

CONTRIBUTING.md

+   to Socket", "src: fix typos in node_lttng_provider.h"
+   - be meaningful; it is what other people see when they
+   run `git shortlog` or `git log --oneline`.<br>
+   Check the output of `git log --oneline files_that_you_changed` to find out


Maybe files_that_you_changed -> files/you/changed to emphasize that it's a path

gibfahn · 2017-03-07T23:14:40Z

CONTRIBUTING.md

 2. Keep the second line blank.
-3. Wrap all other lines at 72 columns.
+3. Wrap all other lines in the body at 72 columns.


in the body seems unnecessary here

I added it for that same reason as #11732 (comment), where it's later referred to as the body.

Okay, then maybe just say Wrap the body at 72 columns, otherwise it's unclear whether there are other lines not in the body that might not need to be wrapped.

bf4 · 2017-03-07T23:44:38Z

@Trott @gibfahn I removed the typo commit entirely, and added a second commit that contains the changes per feedback.

bf4 · 2017-03-07T23:45:29Z

CONTRIBUTING.md

+    Fixes: https://github.com/nodejs/node/issues/1337
+    Refs: http://eslint.org/docs/rules/space-in-parens.html
+    Refs: https://github.com/nodejs/node/pull/3615
+    ```


❓ Maybe also check your commit for spelling and clarity ? :)

Requested changes have been made.

fhinkel · 2017-03-09T09:32:30Z

CONTRIBUTING.md

+   imperative verb. Examples: "net: add localAddress and localPort
+   to Socket", "src: fix typos in node_lttng_provider.h"
+   - be meaningful; it is what other people see when they
+   run `git shortlog` or `git log --oneline`.<br>


I think the html   is a typo here?

🤔 I thought the line break improved readability

I think you can put a blank line in the file to get a break

@sam-github I made the 80 char change, but left the  . Please review my screenshots and let me know which you prefer.

With br without br with blank line

doc: make commit guidelines easier to reference #11732 (comment)

doc: make commit guidelines easier to reference #11732 (comment)

I can't tell the difference, they all look fine

gibfahn · 2017-03-12T14:49:30Z

@bf4 can you change .github/PULL_REQUEST_TEMPLATE.md as #11792 does?

cc/ @sam-github

bf4 · 2017-03-12T19:19:37Z

CONTRIBUTING.md

-   be prefixed with the name of the changed subsystem and start with an
-   imperative verb. Example: "net: add localAddress and localPort
-   to Socket"
+#### Commit Message Guidelines


I was wondering if you think this should make a distinction between the pr title and description vs. the various commits that might make up a PR, including ones from feedback.

I don't think any of this applies to the PR title and description, you can do what you want with them.

It might be worth mentioning that these rules apply to the commit message not to the PR title though, as often people aren't aware of the difference.

Oh, well, I kind of read it as meaning the pr title and description, assuming most prs are squashed. I haven't been following that in my commits since the first one....

bf4 · 2017-03-12T19:23:52Z

@gibfahn Added link from PR Template, cc @sam-github

bf4 · 2017-03-12T19:25:45Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -13,7 +13,7 @@ Contributors guide: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md
 - [ ] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes
 - [ ] tests and/or benchmarks are included
 - [ ] documentation is changed or added
- [ ] commit message follows commit guidelines
+- [ ] commit message follows [commit guidelines](../CONTRIBUTING.md#commit-message-guidelines)


Confirming the ../ will link correctly when the PR is created

Confirming the ../ will link correctly when the PR is created

It won't link properly when you raise a PR, as the PR comes under https://github.com/nodejs/node/compare. You can try it with a draft PR (or by editing this PR's title, but none of the relative links work:

../ commit guidelines

./ commit guidelines

commit guidelines

Had a similar issue here.

EDIT: Looks like you already fixed it!

gibfahn · 2017-03-17T12:56:42Z

@bf4 #11792 has been landed, so this'll need a rebase.

sam-github · 2017-03-23T22:57:52Z

@bf4 nice layout improvements here, can you rebase and re-push?

bf4 · 2017-03-24T01:34:34Z

@gibfahn @sam-github Rebased off of master; Added commit to accommodate #11792

Let me know if you'd like me to squash down to one commit.

sam-github · 2017-03-24T02:06:18Z

CONTRIBUTING.md

+1. The first line should:
+   - contain a short description of the change
+   - be 50 characters or less
+   - be entirely in lowercase with the exception of proper nouns, acronyms, and the words that


wrap to 80 colums

sam-github · 2017-03-24T02:08:25Z

CONTRIBUTING.md

+   imperative verb. Examples: "net: add localAddress and localPort
+   to Socket", "src: fix typos in node_lttng_provider.h"
+   - be meaningful; it is what other people see when they
+   run `git shortlog` or `git log --oneline`.<br>


I think you can put a blank line in the file to get a break

sam-github · 2017-03-24T17:04:39Z

@bf4 squashing to one commit would be helpful, thanks

bf4 · 2017-03-30T22:04:38Z

CONTRIBUTING.md

+   - be meaningful; it is what other people see when they
+   run `git shortlog` or `git log --oneline`.
+
+   Check the output of `git log --oneline files/you/changed` to find out


doesn't look right without the  

bf4 · 2017-03-30T22:11:22Z

CONTRIBUTING.md

+   to Socket", "src: fix typos in node_lttng_provider.h"
+   - be meaningful; it is what other people see when they
+   run `git shortlog` or `git log --oneline`.
+   Check the output of `git log --oneline files/you/changed` to find out


Without the br and just a line break, doesn't do the   either

- Can now link to 'Commit Guidelines' from pull requests - Breaks up commit requirements and recommendations

bf4

@sam-github I made the 80 char change, but left the  . Please review my screenshots and let me know which you prefer. https://github.com/nodejs/node/pull/11732/files#r109050698

I also squashed the commits and rebased against master.

Let me know what else I can do. Sorry for the delay in getting back to you.

benjamingr · 2017-04-16T00:24:27Z

@sam-github are you content with the latest changes?

gibfahn · 2017-04-18T17:31:08Z

@Trott @fhinkel LGTY?

Trott · 2017-04-19T04:20:52Z

@gibfahn No objections from me.

bf4 · 2017-04-21T06:02:22Z

Anything else for me to do?

gibfahn · 2017-04-22T17:59:31Z

Landed in bd97e48

- Can now link to 'Commit Guidelines' from pull requests - Breaks up commit requirements and recommendations PR-URL: #11732 Refs: #11723 (comment) Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

- Can now link to 'Commit Guidelines' from pull requests - Breaks up commit requirements and recommendations PR-URL: nodejs/node#11732 Refs: nodejs/node#11723 (comment) Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 7, 2017

bf4 changed the title ~~<!--~~ CONTRIBUTING: make commit guidelines easier to reference Mar 7, 2017

Trott previously requested changes Mar 7, 2017

View reviewed changes

mscdex added the meta Issues and PRs related to the general management of the project. label Mar 7, 2017

gibfahn suggested changes Mar 7, 2017

View reviewed changes

bf4 force-pushed the clarify_commit_guidelines branch from 85ce194 to 80fa742 Compare March 7, 2017 23:42

bf4 commented Mar 7, 2017

View reviewed changes

fhinkel reviewed Mar 9, 2017

View reviewed changes

gibfahn mentioned this pull request Mar 12, 2017

doc: linkable commit message guidelines #11792

Closed

4 tasks

bf4 commented Mar 12, 2017

View reviewed changes

bf4 force-pushed the clarify_commit_guidelines branch 3 times, most recently from a2a20e5 to 49451a9 Compare March 12, 2017 19:23

bf4 commented Mar 12, 2017

View reviewed changes

bf4 force-pushed the clarify_commit_guidelines branch from 49451a9 to 13d97b8 Compare March 12, 2017 19:29

bf4 changed the title ~~CONTRIBUTING: make commit guidelines easier to reference~~ doc: make commit guidelines easier to reference Mar 12, 2017

bf4 force-pushed the clarify_commit_guidelines branch from 13d97b8 to adbf17f Compare March 24, 2017 01:32

sam-github suggested changes Mar 24, 2017

View reviewed changes

bf4 force-pushed the clarify_commit_guidelines branch from adbf17f to 84200a4 Compare March 30, 2017 22:03

bf4 commented Mar 30, 2017

View reviewed changes

bf4 force-pushed the clarify_commit_guidelines branch from 84200a4 to 17f31e0 Compare March 30, 2017 22:04

bf4 commented Mar 30, 2017

View reviewed changes

doc: CONTRIBUTING: make commit guidelines easier to reference

501d159

- Can now link to 'Commit Guidelines' from pull requests - Breaks up commit requirements and recommendations

bf4 force-pushed the clarify_commit_guidelines branch from 17f31e0 to 501d159 Compare March 30, 2017 22:11

bf4 commented Mar 30, 2017

View reviewed changes

gibfahn approved these changes Mar 31, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

sam-github approved these changes Apr 17, 2017

View reviewed changes

gibfahn closed this Apr 22, 2017

bf4 deleted the clarify_commit_guidelines branch April 24, 2017 02:20

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

gibfahn added the land-on-v6.x label May 16, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

doc: make commit guidelines easier to reference #11732

doc: make commit guidelines easier to reference #11732

Conversation

bf4 commented Mar 7, 2017 • edited Loading

Checklist

Affected core subsystem(s)

bf4 commented Mar 7, 2017

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

bf4 commented Mar 7, 2017

bf4 Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Mar 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Mar 12, 2017

Choose a reason for hiding this comment

gibfahn Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

gibfahn commented Mar 17, 2017

sam-github commented Mar 23, 2017

bf4 commented Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 left a comment

Choose a reason for hiding this comment

benjamingr commented Apr 16, 2017

gibfahn commented Apr 18, 2017

Trott commented Apr 19, 2017

bf4 commented Apr 21, 2017

gibfahn commented Apr 22, 2017

bf4 commented Mar 7, 2017 •

edited

Loading

gibfahn Mar 7, 2017 •

edited

Loading

bf4 Mar 7, 2017 •

edited

Loading

bf4 Mar 12, 2017 •

edited

Loading

bf4 Mar 30, 2017 •

edited

Loading

gibfahn Mar 12, 2017 •

edited

Loading

bf4 commented Mar 24, 2017 •

edited

Loading