feat: validate commit message for 1 commit PRs #87
Conversation
There was a problem hiding this comment.
Hi @simlrh,
thank you for starting the discussion around this issue and this corresponding pull request. I noticed this behaviour of Github as well and was a bit put off by it.
I was initially a bit opposed to adding this, similar to how you mentioned, since it increases the scope of the action a bit, but since you made it opt-in, I guess it's just a pragmatic workaround.
I also saw that a similar tool like this, semantic-pull-requests, also discussed this issue in zeke/semantic-pull-requests#17 and fixed it in zeke/semantic-pull-requests#105.
FWIW this seems to be addressed in refined-github: refined-github/refined-github#423, but it might be difficult to force all reviewing maintainers of a given repository to use that extension.
So long story short, I think this is a good addition! Thank you so much for taking the time to implement it carefully 👏.
Please consider the two comments I added.
| # will suggest using that commit message instead of the PR title for the | ||
| # merge commit, and it's easy to commit this by mistake. Enable this option | ||
| # to also validate the commit message for one commit PRs. | ||
| validateSingleCommit: true |
There was a problem hiding this comment.
Thank you for including good docs and testing the feature!
src/index.js
Outdated
| subjectPattern, | ||
| subjectPatternError | ||
| }, | ||
| 'single commit message' |
There was a problem hiding this comment.
Nit: I'd move the messageType inside the second options argument. I'm using this object as a workaround for named arguments as I think it helps a bit with clarity, making it clear at the call site what the argument represents.
But the comment below is closely related to this as well.
src/validateCommitMessage.test.js
Outdated
| await expect( | ||
| validateCommitMessage('Fix bug', undefined, 'single commit message') | ||
| ).rejects.toThrow( | ||
| 'No release type found in single commit message "Fix bug".' |
There was a problem hiding this comment.
Currently the full error message looks like this:
No release type found in single commit message "Validate commit message for 1 commit PRs (and test option)". Add a prefix to indicate what kind of release this pull request corresponds to (see https://www.conventionalcommits.org/).
I'm not sure this helps the PR author to address the issue since we say "Add a prefix".
Maybe we should clarify that we're working around an issue of Github here, possibly referencing https://github.mirror.nvdadr.community/t/how-to-change-the-default-squash-merge-commit-message/1155 and telling the user to add another commit to avoid the issue? That's also what semantic-pull-requests does.
Since the error message becomes a bit different here, maybe it makes sense to pass in an enum value to validateCommitMessage that can be identified and used to alter the message as necessary.
E.g.
validateMessage('foo', {type: 'title'})
validateMessage('foo', {type: 'commit'})I've also renamed to function again here, since it can be either a commit message or a PR title.
What do you think?
There was a problem hiding this comment.
I don't want to complicate the logic in validateMessage too much - how about I revert the changes I made there, and use a try/catch in index.js to replace any single commit error with a catch-all message for the problem?
|
🎉 This PR is included in version 3.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Adds an option to validate the commit message when a PR only contains one commit. Closes #86
When a PR only contains one commit, GitHub suggests that commit message for the "Squash and merge" commit instead of the PR title. This option fails the check if the commit message isn't a conventional commit, preventing an accidental merge of a non-conventional commit into the main branch.
Here's the PR from this branch to the fork's main branch. It passes since the option is disabled by default.
Demo PRs with the option enabled: