Hi everyone, currently it's not always clear which pull requests needs another review. Especially when there are several iterations of changes during a review.
Currently, once a pull request is ready for a review we assign the label
Needs Review. Then someone reviews it and the issue stays in
Needs Review until merged. This makes it hard to know when the issue needs yet another review. You often have to click into the same issue several times to check if there's an update and if it actually does need another review or if we are waiting for the author to make changes.
I propose that once an issue has been reviewed, we remove the
Needs Review label, and maybe add some other label like
WIP work in progress or
Changes Requested or change it to draft or simply don't add another label. Then, once the PR author made the required changes the label
Needs Review would be added again.
I think this would make it more clear which reviews actually need our attention currently since reviews have pretty much highest priority for us.
btw maybe the github review feature could be leveraged? eg. https://docs.github.com/en/github/managing-your-work-on-github/filtering-pull-requests-by-review-status (i'm not sure if it meets the needs but maybe it could)
:+1: could help. would make things clearer for me at least (though it's not a big time sink for me atm to just look through all the PRs in the current milestone and see what can be merged/what needs reviews)
Sounds fine I guess.
we could maybe also use the review states more actively. Once the required changes have been done by the author it might maybe also be helpful to dismiss reviews that should have been solved with new commits. If multiple peoples are doing reivew we might otherwise end up with having multiple states from different reviewes within one PR.
@diosmosis comment about the milestone is very helpful too:
for me atm to just look through all the PRs in the current milestone and see what can be merged/what needs reviews
It means if I work on something which has a milestone in the future but finished earlier, I should set the milestone to the very next one?
I like the idea to set/unset labels. As the team grows, more and more active PRs will have, this could really help quickly go through.
@flamisz no, but it would help to set the milestone to the same one as the issue. What goes into the next milestone has to do with the value a change might bring & the potential cost of introducing it into live systems & making it stable, rather than what is coded and ready to go at any given moment. And @mattab & @tsteur are the ones in charge w/ prioritization at this level, so we wouldn't change that unless we knew it was a valuable enough or a small enough change.
trying to summarise the thoughts so far from previous comments:
Does this overall sound in the right direction? Any thoughts / objections? Also of course we can change/improve things any time .
Overall the goal again be to easily be able to see which issues currently need a review etc and trying to get approved PRs merged as quick as possible (for example so less potential merge conflicts happen etc)
I like it, it's a very clean and simple workflow.
Great, I've put this into the docs and I'll close this for now and we can change it any time. Any feedback any time be greatly appreciated. :)