-
Notifications
You must be signed in to change notification settings - Fork 59
feat(ng-dev): add conditional autosquash merge strategy #3169
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
Conversation
9400cf7 to
4528a64
Compare
4528a64 to
2f25a04
Compare
2f25a04 to
4f576d1
Compare
bb5e914 to
30def70
Compare
Introduces a new conditional autosquash merge strategy. This strategy uses the autosquash merge strategy if the pull request contains fixup or squash commits, and the GitHub API merge strategy otherwise. This allows pull requests that do not need to be squashed to be closed as "merged" on GitHub, rather than "closed".
30def70 to
fcf8702
Compare
josephperrott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall its looking good, just want to make sure we are certain about the one if block.
| // Squash and Merge will create a single commit message and thus we can use the API to merge. | ||
| if ( | ||
| method === 'rebase-with-fixup' && | ||
| (pullRequest.needsCommitMessageFixup || (await this.hasFixupOrSquashCommits(pullRequest))) | ||
| ) { | ||
| return super.merge(pullRequest); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why but I just cannot parse this block to save my life.
I think we don't want to perform the merge immediately if it needs commit message fixing right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comit fixing is handled in
| override async merge(pullRequest: PullRequest): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't realize/connect that it changed base classes.
josephperrott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
This PR was merged into the repository. The changes were merged into the following branches:
|
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Introduces a new conditional autosquash merge strategy. This strategy uses the autosquash merge strategy if the pull request contains fixup or squash commits, and the GitHub API merge strategy otherwise.
This allows pull requests that do not need to be squashed to be closed as "merged" on GitHub, rather than "closed".