doc: clarify expectations for PR commit messages - #30922
Conversation
Sorry, something went wrong.
MylesBorins
left a comment
There was a problem hiding this comment.
Lgtm
Sorry, something went wrong.
|
Fast track? |
Sorry, something went wrong.
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
Sorry, something went wrong.
There was a problem hiding this comment.
Would it make sense to set up a make task (phony target) specifically for this scenario?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
|
Sorry, something went wrong.
|
Keeping the fully qualified reference helps making sure the PR is in good shape, w.r.t commit message; but puts more effort on the contributor's side - especially increases the entry barrier for new comers (needing to install the core-validate-commit module etc.). Preferring content over processes and keeping an aim to foster fast onboarding, I am fine with removing the reference altogether. |
Sorry, something went wrong.
|
I'm opposed to fast-tracking this. There's no rush and we might as well get it right the first time. |
Sorry, something went wrong.
|
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 --no-validate-metadata to avoid the problem described here. |
Sorry, something went wrong.
|
Although I find Gireesh's argument compelling, so maybe removing it is really the way to go?
|
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
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 <myles.borins@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <myles.borins@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <myles.borins@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <myles.borins@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Prior to this commit, new contributors were suggested
to use a utility to validate commit messages. Although
not inaccurate, this utility produces misleading results.
Checklist
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