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

Improve review labels for PRs #3155

Open
apinnick opened this issue Jul 21, 2024 · 8 comments
Open

Improve review labels for PRs #3155

apinnick opened this issue Jul 21, 2024 · 8 comments

Comments

@apinnick
Copy link
Contributor

apinnick commented Jul 21, 2024

I find the current PR labels "Not yet reviewed" confusing because it is assigned automatically and is not updated. This means that it is not easy to see which PRs actually need to be reviewed.

Can we ask reviewers to update the label to "review done"? "Review in progress" is also useful.

@ekohl
Copy link
Member

ekohl commented Jul 22, 2024

It does get automatically updated, but I think few reviewers actually request changes and only submit a review

@Lennonka
Copy link
Contributor

@ekohl Could it get updated also when someone just submits a review?

@ekohl
Copy link
Member

ekohl commented Jul 22, 2024

I have thought about that before and think it makes sense. It's been on my todo list for ages to write up a description of how it works. Perhaps this is a good excuse to do so

@apinnick
Copy link
Contributor Author

apinnick commented Jul 22, 2024

It does get automatically updated, but I think few reviewers actually request changes and only submit a review

@ekohl Are you saying it gets updated only if reviewers request changes? Not when a review is submitted?

That does not make sense. "LGTM" should count as a review.

@ekohl
Copy link
Member

ekohl commented Jul 23, 2024

Example of where the bot is acting: theforeman/foreman#10245 (review)

It's important to know that there are 3 ways to submit a review:

  • Approved
  • Comment
  • Request changes

Keeping that in mind when reading the code you should notice we only act on Approved and Request changes. I practice I often see use the second one: comment. We can easily start treating that the same as Request changes, but we don't do that today.

@apinnick
Copy link
Contributor Author

Keeping that in mind when reading the code you should notice we only act on Approved and Request changes. I practice I often see use the second one: comment. We can easily start treating that the same as Request changes, but we don't do that today.

I also tend to use "Comment" in practice. I'm not sure why it would be desirable to treat "Comment" differently from "Approved" and "Request changes". Could we add this to the workflow?

@ekohl
Copy link
Member

ekohl commented Jul 23, 2024

It was introduced in theforeman/prprocessor@dcc45c7. There's a very good chance that was right when the feature was introduced (I still recall using GH without the reviews feature and we just used inline comments) and things were less clear back then. I don't think it's been really discussed that much in the past (almost) 8 years.

@ekohl
Copy link
Member

ekohl commented Jul 23, 2024

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

No branches or pull requests

3 participants