doc: clarify expectations for PR commit messages#30922
Uh oh!
There was an error while loading. Please reload this page.
Conversation
MylesBorins commented Dec 12, 2019
Fast track? |
Prior to this commit, new contributors were suggested to use a utility to validate commit messages. Although not inaccurate, this utility produces misleading results. * Remove reference to `core-validate-commit`
| See [core-validate-commit](https://github.com/nodejs/core-validate-commit) - | ||
| A utility that ensures commits follow the commit formatting guidelines. | ||
There was a problem hiding this comment.
If we want to keep a reference to core-validate-commit then we should instruct to run with --no-validate-metadata as we do in Travis for pull requests.
node/tools/lint-pr-commit-message.sh
Line 38 in 357a992
There was a problem hiding this comment.
Would it make sense to set up a make task (phony target) specifically for this scenario?
There was a problem hiding this comment.
Ideally, the make test target should include a commit tester... though I suspect there are some complications, like only linting the new commit messages, not every one in the history of node.js,and perhaps dealing with fixup commits, etc.
There was a problem hiding this comment.
@sam-github, are you saying that this check should take place when running the command mentioned in Step 6: Test? If so, simply validating the most recent commit should be all that is necessary as prior commits can be safely assumed to be valid (having already landed in upstream master). It's probably important to keep this discussion within the scope of ensuring a successful process for those following this guide.
codecov-io commented Dec 13, 2019
Codecov Report
@@ Coverage Diff @@## master #30922 +/- ## ======================================= Coverage 88.31% 88.31% ======================================= Files 184 184 Lines 62722 62722 ======================================= Hits 55391 55391 Misses 7331 7331Continue to review full report at Codecov.
|
gireeshpunathil commented Dec 13, 2019
Keeping the |
Trott commented Dec 13, 2019
I'm opposed to fast-tracking this. There's no rush and we might as well get it right the first time. |
Trott commented Dec 13, 2019
I'm OK with removing this, but I'd prefer we leave it an add a note that end-users will probably want to run it with |
Trott commented Dec 13, 2019
Although I find Gireesh's argument compelling, so maybe removing it is really the way to go?
|
This comment has been minimized.
This comment has been minimized.
Trott added the author ready 
Prior to this commit, new contributors were suggested to use a utility to validate commit messages. Although not inaccurate, this utility produces misleading results. * Remove reference to `core-validate-commit` PR-URL: #30922 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>
BridgeAR commented Dec 15, 2019
Landed in b428140 🎉 |
Prior to this commit, new contributors were suggested to use a utility to validate commit messages. Although not inaccurate, this utility produces misleading results. * Remove reference to `core-validate-commit` PR-URL: #30922 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Prior to this commit, new contributors were suggested to use a utility to validate commit messages. Although not inaccurate, this utility produces misleading results. * Remove reference to `core-validate-commit` PR-URL: #30922 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Prior to this commit, new contributors were suggested to use a utility to validate commit messages. Although not inaccurate, this utility produces misleading results. * Remove reference to `core-validate-commit` PR-URL: #30922 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]>
10 participants
Prior to this commit, new contributors were suggested
to use a utility to validate commit messages. Although
not inaccurate, this utility produces misleading results.
core-validate-commitChecklist
The last line in section Step 4: Commit of the Contributing Pull Requests guide suggests using a utility to validate commit messages, namely
core-validate-commit. The following is what results when running the tool against a commit that has followed the guide (and even went too far by adding its PR-URL).As can be seen in the snip above, the utility produces misleading results that may inadvertently lead new contributors to think that they, themselves are required to include this metadata in their PR's commit message when in reality, omitting this metadata will not cause the CI checks to fail. See how this leads to the confusion experienced here: #30216 (comment).
Although this PR is removing reference to this tool entirely from this document, I hope that it leads to a constructive discussion about a preferred way of making this clarification (if it is deemed that this is not ideal).
/cc @MylesBorins