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

feat(options): don't remind if change requested #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(options): don't remind if change requested #4

wants to merge 1 commit into from

Conversation

leizmonk
Copy link
Contributor

Story

This feedback came from one of the engineers on my team where I'm using this. He was continuing to be bothered with reminders on a PR he had been requested to review even after he reviewed it and had requested changes of the PR author. This check should stop that from happening.

if(b.state == "APPROVED")
return true;
if(b.state == "APPROVED" || b.state == "CHANGES_REQUESTED")
return true;

if(b.submitted_at < pullRequest.updated_at)
Copy link
Owner

Choose a reason for hiding this comment

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

@leizmonk this won't work .. If you requested changes you shouldn't be notified unless your changes were addressed by the author .. checking this is complicated and so line 153 makes sure that you are notified if the pull request has changed after you submitted a review and does not notify you otherwise ...

If we wish to enhance it we need to think of a way to know that these changes address the reviewer requested changes and not those of another reviewer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ahmadposten if line 153 is intended to not notify you if the pull request is in "Change requested" status and the author of the PR hasn't addressed the request, it's not working properly. Engineers on my team were getting notifications on PRs they had been requested to review even after requesting changes of the PR author.

Because there's no consistent way to guarantee that a PR author addressed the changes requested reviewer (ex: on my team people usually @ the reviewer on the review comment thread when they've pushed a change to address the reviewer's feedback, but that might not be the case on other teams), I might just close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants