Clarify the PR review process

[benoit74]

Clarify the PR review process

[mgautierfr]

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

[benoit74]

External links where more about people saying this is not an easy subject to tackle, that it is always a subject of debate, and that the most important thing to do is to have a clear, written, shared process. Job's done ^^

I pushed changes to https://github.com/openzim/overview/wiki/openZIM-workflow

Please review it with https://github.com/openzim/overview/wiki/openZIM-workflow/_compare/c6e8f827483b4a1605e0a4b82bb2522669eae9fd...92b303a36b981a3f3a40e62b4177a7fff7a2787d

And do not hesitate to speak-up / amend if this does match your expectations.

[rgaudin]

I agree with the proposal, augmented with @mgautierfr's comments.

[benoit74]

I like a lot your suggestions @mgautierfr, thank you!
Let's wait for a feedback from @kelson42 and I will update the doc.
Is it ok for you if I duplicate this documentation in both locations mentioned in the original issue?

[kelson42]

Sorry for the late feedback. I don't have red the external links because of lack of time but I assume they are full of good advices. For the rest, I believe to agree with everything which has been written, comments included. Please update the appropriate wiki pages. @benoit74 Thank you for helping improving/clarifying the documentation on this very specific point.