Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Single commit validation does not factor in merge commits [#108] #131

Merged
merged 3 commits into from
Oct 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .github/workflows/lint-pr-title-preview-validateSingleCommit.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: 'Lint PR title preview (current branch, validateSingleCommit enabled)'
on:
pull_request:
types:
- opened
- edited
- synchronize

jobs:
main:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
with:
node-version: 12
- run: yarn install
- run: yarn build
- uses: ./
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
validateSingleCommit: true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem!

37 changes: 29 additions & 8 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,37 @@ module.exports = async function run() {
});

if (validateSingleCommit) {
const {data: commits} = await client.pulls.listCommits({
owner,
repo,
pull_number: contextPullRequest.number,
per_page: 2
});
const commits = [];
let nonMergeCommits = [];

if (commits.length === 1) {
for await (const response of client.paginate.iterator(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty cool with the iterator!

client.pulls.listCommits,
{
owner,
repo,
pull_number: contextPullRequest.number
}
)) {
commits.push(...response.data);

// GitHub does not count merge commits when deciding whether to use
// the PR title or a commit message for the squash commit message.
nonMergeCommits = commits.filter(
(commit) => !commit.commit.message.startsWith('Merge branch')
);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, another minor nit: I think we don't need commits, but only nonMergeCommits – right? Now we're applying this filter on every iteration step.

Maybe something like this would be a bit simpler?

if (!response.data.commit.message.startsWith('Merge branch')) {
  nonMergeCommits.push(response.data);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally think that's a micro optimization. I also find the existing solution cleaner, and gives us the ability to take actions on the whole list of commits in the future if need be.

If you'd like to switch it though, feel free. It's your repo so I respect the decision. Both should work just fine.

One note is that if you implement your example, you'll need to make sure to spread response.data (nonMergeCommits.push(...response.data).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fair enough – let's go with your approach and avoid more changes & testing.

Thanks again!


// We only need two non-merge commits to know that the PR
// title won't be used.
if (nonMergeCommits.length >= 2) break;
}

// If there is only one (non merge) commit present, GitHub will use
// that commit rather than the PR title for the title of a squash
// commit. To make sure a semantic title is used for the squash
// commit, we need to validate the commit title.
if (nonMergeCommits.length === 1) {
try {
await validatePrTitle(commits[0].commit.message, {
await validatePrTitle(nonMergeCommits[0].commit.message, {
julianpoy marked this conversation as resolved.
Show resolved Hide resolved
types,
scopes,
requireScope,
Expand Down