The issue has been closed

Warn about auto-merge being enabled when approving a PR #4257

karfau posted onGitHub

I often already approve PRs even though I would prefer to get answers to some of my questions in the review. Maybe I should just comment in that case, but well... It regularly happens to me that the PR is landed right away (or some seconds/minutes later) because the PR has been configured to be merged/squashed/rebased automatically.

I think it would be helpful to see some indicator in the "Finish your review" dialog of the Files tab, that makes you aware that the PR will land as a result of your approval.

Since it might be more complex to check the current status and it might change to that state in a few seconds or minutes, in case auto merge is enabled for the PR, I think it makes more sense to just check for that toggle.

Include a REAL URL where the feature should appear. e.g. Do you want a feature to appear on the main page of a repo? Paste a link to a repo

I'm not sure what "REAL URL" I should add, the dialog that has the "Approve" button (with quick-review-buttons being active, otherwise option) is only visible for maintainers of a repo and only after a click.

According to the comment below the affected URL would be https://github.com/{org_or_user}/{repository}/pull/{id}/files

Is there a way for us to know without an api call that auto-merge is on?

posted by yakov116 almost 4 years ago

I have not done any coding for this addon, the status is visible on the "Conversation" tab and from the behavior I observed between those tabs it some times seems, that they at least have some behavior that looks like a single page app... not sure if that helps.

On the other hand, maybe there is already some existing API call that provides this information? (Or are API calls for each feature separated?)

posted by karfau almost 4 years ago

Being a new GitHub feature if you ping some producer or the CEO on Twitter you might have a good chance at seeing this natively.

Here I think the main issue is finding this piece of information. Perhaps if it’s available on the PR Conversation tab we can cache it preemptively. If someone clicks that automerge while you’re reviewing we can’t find out without polling continuously, which we can’t do.

posted by fregante almost 4 years ago

I'm not sure what "REAL URL" I should add, the dialog that has the "Approve" button (with quick-review-buttons being active, otherwise option) is only visible for maintainers of a repo and only after a click.

GitHub changes for everyone, this is implied. The URL in this case would have been a link to the Files tab in a PR… I’d assume.

posted by fregante almost 4 years ago

That's a good idea ~~, do you have a specific person in mind? I would need to some (most likely short) research on the topic.~~

Update: https://twitter.com/cb_karfau/status/1384248730203820042

posted by karfau almost 4 years ago

If someone clicks that automerge while you’re reviewing we can’t find out without polling continuously, which we can’t do.

You are right, timing can cause it to happen. I guess the most reliable way would be to check before clicking the button (would need to be both the optimized "Approve" button and the original one from github) and in case auto merge is active (and wasn't indicated before?) show the indicator and return false so the user has to click twice...

Sounds much more complex than I initially thought :)

I will anyways try to ping somebody from github publicly. (Update: Done)

posted by karfau almost 4 years ago

By the way we could find out whether auto-merge is enabled, but we don’t know if it will be triggered by the current review or not. Would it be useful?

Also my understanding is that auto-merge needs to be manually clicked in each PR, is that correct?

posted by fregante almost 4 years ago

Yes, everything you said us correct.

And the more I think about it, the less I think it is a valid way to think about it.

When I approve it, somebody else could still set it to auto-merge afterwards, potentially even without seeing my comments.

So I think it's a better option to only put a comment review, when I don't want the PR to be merged.

posted by karfau almost 4 years ago

So I think it's a better option to only put a comment review, when I don't want the PR to be merged.

That's a good point!

posted by fregante almost 4 years ago

Fund this Issue

$0.00

Funded

Pull requests

Warn about auto-merge being enabled when approving a PR #4257

Company

Community

Support

Connect