mgautierfr
commented
1 year ago We indeed don't have a specified review process. But I mostly agree with your proposition. With few comments:
PR must not be approved until the reviewer is happy with the code / conversations Agree no conversation must be left unresolved before the approval is given by the reviewer I'm not sure here. Approving the PR, is a implicit agreement on what has been done on code / conversations. We may still have open question on a subject while we agree on the code and the PR can be merged.
it is the reviewer responsibility to resolve conversations authors must not resolve conversations, except for obvious code suggestions that have been applied to the code base
I would say that the last person to put a comment must not resolve the conversation. (Except if this is a clear and "easy" approval)
once the PR is approved it means that the author can merge
I like the idea that the author do not merge the PR. It is a nice way to share responsibility on the code and it avoid "forced" merge (or totally unreviewed code)
author must not change the code once the approval is given, at least not without requiring a new approval (but this should be a rare case, normal process is to merge asap to spread the change, and open a new PR for new changes ; this only makes sense if the merge makes no sense, e.g. because a very significant bug has been discovered)
Totally agree. But this rule is less important if we say that author cannot merge the PR.
should something still have to be discussed in a conversation but is not blocking for the change to be merged, an issue must be open to track the discussion point and the conversation will be resolved (reviewer can explicitly ask the author to do it, or the author can suggest it in a conversation)
Agree
rgaudin
benoit74
kelson42
I need you to help me clarify the PR review process, because it is too blurry for me and causing frustration.
I do not find enough precision in kiwix/overview/CONTRIBUTING.md or in openzim/overview Wiki. Please point me to the correct direction if I missed something.
This is a mid-term enhancement, I hope to reach a conclusion on this within few weeks, but there is clearly no hurry.
This issue describes below the situation(s) that occurred between Renaud and I to give a background, but this is not a personal conflict (at least from my position š ).
What happened
I've once again failed to understand the PR process and caused frustation to Renaud by resolving a conversation too soon (this time) : https://github.com/offspot/metrics/pull/25#discussion_r1332692481
I had understood that once PR is approved, I have to resolve all conversations on my own because they are just comments to raise my awareness should I want to fix this. But obviously this is not always the case. And I find that it is weird to have pending conversation but an approved PR.
In contradiction to that, on an unapproved PR Renaud asked me to stop waiting for him on unresolved conversations and resolve the conversations on my own once the requested code change is done, but I'm always hesitating to do that because I often have doubt whether I got the code change request properly and whether I did the right change.
In the past, the situation already occurred that Renaud was disappointed by a change I've made after his PR approval and explicit request to resolve conversation and proceed with the merge once the change is done. This was exactly because I didn't applied the change / understood the change request properly. I consider that misunderstanding is normal.
All this is very frustrating for all of thus, and I would really like to come up with a much simpler process / rule(s). Simpler meaning, for me, less subject to personal interpretation.
Background research
Luckily (or not), we are not the only ones to struggle on this topic (I just did a very fast Google search):
Proposition
My process proposition is:
This has some drawbacks of course (probably more work for the reviewer), but it is way clearer for me.
I would like also to add some resources to read as a PR author:
And other to read as a PR reviewer:
Suggestions / feedbacks welcomed!