Don't comment again in "awaiting changes" state

[ncoghlan]

After the review that prompted python/cpython#3077 (comment), I did a second review where I replied to some of Yury & Serhiy's comments.

Bedevere picked that up as a new review and repeated its comment: python/cpython#3077 (comment)

Without knowing the details of how Bedevere works, I'm guessing the simplest way of avoiding that would be to skip commenting if the "awaiting changes" label is already set.

[brettcannon]

Probably. I also don't know how often people will leave two consecutive reviews before the reviewer has had a chance to address them either. There's also a question of whether a message per review is useful if e.g. you had asked for changes, and then Yury did, and then Serhiy. One message is fine, but three messages gets the point across there's a lot of interest and work to be done on the PR.

[terryjreedy]

Nick, I don't understand the concrete change you are proposing. We want good reviews. If multiple reviews confuse the bot, so what? I don't always have time to review everything in one sitting. And contributors cannot always respond before I have more to say. It would help if comments could be added to an existing review, but that is out of our control.

Syncosmic's comment "Looks like bedevere-bot needs set, not list..." identifies an obvious change that would improve messages.

[brettcannon]

Nick noticed that because he left to "changes requested" comments before the PR creator did anything, the bot left two comments with instructions on how to notify him that the PR was ready for another review.

The "set, not list" comment was because when the PR creator signaled he was ready for another review it listed various core devs multiple times in the notification comment (and has been fixed).

[ncoghlan]

My concern is with the contributor experience of folks potentially feeling like the bot is "yelling" at them, regardless of whether the reason for multiple requests for changes is one reviewer adding multiple reviews, or multiple reviewers making separate requests.

Now, one thing I did learn is that if we mark a follow up review as a Comment, GitHub still leaves our prior review status in place, which means we can suggest to core devs that it we don't want Bedevere to post again, we can just make sure we flip the review state on the follow-up review.

However, that still feels like a workaround for a misbehaviour in the bot - the main value of Bedevere's comment is to provide inline documentation of what a contributor needs to do to advance the review process, and repeating those docs multiple times on a single PR doesn't seem particularly valuable.

[brettcannon]

Now I'm not so clear on what you're after then. Do you want just pairing of review-needed/ready-for-review comments, or just one review-needed comment ever? I can understand the former but not the latter as the comment still acts as a very visible signal that there's been a "changes requested" review since the last time you thought you were done.

[ncoghlan]

This issue is specifically for not commenting again when the PR is already in the "awaiting changes" state.

Shortening the comment (rather than skipping it entirely) with a phrase like "See [link to previous comment] for more details" when it's a repeat would also be nice to have, but I'd consider that a separate RFE.

[brettcannon]

OK, the bot will now skip leaving another comment about requested changes if "awaiting changes" is already set. I also made the comment more generic by not mentioning any specific core dev.