doc: clarify expectations for PR commit messages #30922
Conversation
|
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 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.
|
|
Keeping the |
|
I'm opposed to fast-tracking this. There's no rush and we might as well get it right the first time. |
|
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 |
|
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]>
|
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]>
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