Github "Changes requested" in an PR completely misleads

GitHub Help How to use Git and GitHub
#1

Hello,

GitHub’s “Changes Requested” message makes it hard to review changes that were already made based on a reviewer’s message.

https://github.com/ARM-software/lisa/pull/492#issuecomment-377962295

The confusion is for 2 reasons:

  1. The commits from the Changes requested (that were made to address the request) appear BEFORE the “Changes Requested” message.

  2. The Changes Requested is big bold and red.

Any chance we can fix this so its more meaningful? The latest commit updates should appear at the bottom. So if I scroll to the bottom of the page, it should show the changes/commit made to address the request, and the tone of “Changes Requested” should be reduced.

14 Likes
#2

Hi @joelagnel,

Thanks for this feedback! We’re always working to improve GitHub, and we consider every suggestion we receive. I’ve logged your feature request in our internal feature request list. Though I can’t guarantee anything or share a timeline for this, I can tell you that it’s been shared with the appropriate teams for consideration.

Please let me know if you have any other questions.

Cheers!

1 Like
#3

I agree that big red letters of “Changes Requested” doesn’t really look nice when the changes are already done.

Maybe another possible solution is to have a “Changes Implemented” state which the author of the change can set when the author feels that the changes are done as requested.

23 Likes
#4

After a PR author clicks the refresh icon button to ask a reviewer to re-review the pull request, the PR status remains as “Changes Requested”.

As the reviewer, I do see the PR now on https://github.com/pulls/review-requested so I can use that for my workflow.
However if I’m on the repo page “Pull requests” tab there’s no indicator as to which PRs are ready for re-review. I think you should indicate this somehow.

Thanks in advance.

6 Likes
#5

As an author, I agree that it would be nice to have some way to resolve the requested changes, similar to how you can resolve a comment thread?

#6

Is this still an open issue ?

#7

I still have the issue at least at my end (since after I’ve made all the necessary changes it still claims that there is more changes to be made :man_shrugging:).

image
image1618×98 6.53 KB

1 Like
#8

The text says
“Merging is blocked
Merging can be performed automatically once the requested changes are addressed.”

Yet, when you click resolve conversation on each of the requested changes, it does not allow merging. You have to re-request the review and have them approve it. It would be nice to have the option at the repo level to give some privileges to merge once the changes have been made. This slows down our workflow.

#9

This issue still exists and it absolutely prevents from working meaningful with PR’s and reviews. It’s impossible to know which PR needs attention from me after someone implemented the requested changes. Even after requesting a re-review it stays on “changes requested” flag.

The only way I know about a re-review is because I get notifications about it or my co-workers let me otherwise know that they finished the change requests and are waiting for me.

But the listing and filtering is completely broken because of that.

It’s very annoying.

#10

It probably shouldn’t in all repo’s I think that needs to be a setting on the repo. Setting 1 (aka anyone can resolve comments). Setting 2 (aka reviewer required to resolve comments).

In Setting 1. I make a code change and then you request changes then you should be the one deciding that any further code updates meet the review comments. As it currently stands I can disagree with comments and simply resolve the conversation. Should that allow the change to be merged (because all the comments are “resolved”)

In Setting 2. In this scenario the person who requested the change is the one who confirms that the change had been meet. I fully agree that there should be a way for the committer to provisionally mark a requested change as “done” or “addressed”, but that should notify the reviewer to review changes from the commit of the previous review onwards and then accept the changes.

Most codebases will opt for setting 1, however some code bases are complex and so a tighter review process is needed, hence setting 2.