doc: Add review document draft

[lutangar]

At last, here's the long-awaited documentation!

[lutangar]

Feel free to add more commits to this, I left spelling mistakes just for you @christpet

I can help make sense of the Github editor if needed

[JalilArfaoui]

draft 1.1 :-)

[JalilArfaoui]

Also, we try to keep commits small, and only include the smallest amount of change that keeps it atomic. In other words : let’s create commits that only do one thing.

[JalilArfaoui]

Not sure I like the recommandiation, as it contradicts my recommandation suggested above …

[christpet]

Hm what's the best way to do this? Should we keep commits atomic or squash the branch history together into one big commit?

[lutangar]

Well maybe, this is just a vocabulary issue (atomic?) or maybe interpretation?

see the discussion below regarding a "commit message guideline" :
https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53#gistcomment-2936929

Sometimes you just a go into a commit frenzy and reach a point where you need overwrite your history.
All those commits are related to the same feature and could be described by single simple sentence.
Feels atomic to me... or mb molecular? :D
Well I don't really care about which is the smallest building block here. What matters is those changes could be sum up with just a few words.

One thing I'd like to avoid is having commits which don't bring any value:

Add `sum` function to `math.js` 

even if I probably did similar things somewhere in the past

[JalilArfaoui]

I think we agree here @lutangar
It’s maybe just a matter of wording indeed
Let’s say that if you PR is small enough and only does one thing, you can always squash :-)

[JalilArfaoui]

Paragraph proposal :

## Assignment / Merge

We consider that the PR author is responsible for it during its lifecycle. It means that the author :

• decides who to require a review from (but everyone can review/comment)
• decides when he got sufficient feedbacks
• decides whether to handle or not non-blocking comments
• decides when to merge, and triggers it

But then again, even if he has the final saying, we usually try to work a team, discuss things and find consensus.

[christpet]

I like this, it's really helpful for the PR author to know that they are the one responsible for their PR.

Just so I understand, the PR author does not have the final say on merging something to main, correct? That responsibility should be with one of the reviewers right? I'm just thinking of a potential case where a PR author wants to merge but there are clear issues with the PR. In that case, it's best if someone with knowledge of the system makes the final call. What do you think?

[lutangar]

Discussed with @felix-lambert last week, and what he suggested kind of make sense:

• if the reviewer approved the PR, he should be the one to merge it

Note: it avoids one extra cycle whereas the PR is approved and can be merged

Note 2: Actually we kind of did that already but not always, I'm not really sure of the unwritten rule behind this.

But what about when there are 2 reviewers or more then? Is one approval still sufficient?
Shall it be a rule? An exception?

[JalilArfaoui]

But what about when there are 2 reviewers or more then

• That’s why I suggested that the PR author is the one in position to know if he got enough feedback from review(s) or not, and decide to merge. Even if I get how it adds a «cycle» …

• It also solves the problem of considering if the remaining discussions are blocking or not

Of course, this only works assuming that PR author is experienced enough to be able make this decision.

In fact, we could adopt my proposal if the PR author is the maintainer,
But if the author is not a maintainer, different rule : Any maintainer can merge.

[christpet]

Yes!

[lutangar]

Could you create issues in the respective repositories?
I'm curious to see how you would fill in the "feature request template" for this.

[christpet]

Made some typo/wording fixes in a batch of suggestions - I think you can just commit them all at once if you like them?

[lutangar]

Made some typo/wording fixes in a batch of suggestions - I think you can just commit them all at once if you like them?

Loved it! Didn't find an option to commit them all at once tho'. But I'll rewrite history to make it look like so once we're all set.