Ask a question: Review-and-merge policy

[sshine]

(Note: I accidentally hit Ctrl+Enter in the edit window, which apparently submits the form.)

It seems that this is at least the third repository in my time at Exercism that sees a lot of cross-track activity. The other repositories that I'm comparing this repository to are website-copy that has drowned in pull-requests and problem-specifications that has drowned in disagreement.

With 20+ pull requests opened in the last 24 hours and 17 open right now, we are soon to decide which of our strategies will be picked. If neither seem satisfactory, now would be a good time to come up with an alternative.

So for example @tehsphinx submitted some typo fixes. I had no problem just approving and merging those, even though they were addressed to @ErikSchierboom. He'll see them and there's no conflict. But my own first pull request, #50, I must have somehow missed a formatting rule. Not sure it's really a rule as much as emergent behavior that @tehsphinx had noticed and I hadn't, so great synergy. But then @bergjohan submits #81 where ## Contributing Maintainers is removed. I thought "Huh, there's no precedence here. I think I'll just approve that, and do it myself in #50, and this must constitute emergent behavior." I merged it and @bergjohan eventually changed his mind, and so did I (keeping the headline and "Not yet." below it).

54 is pretty clearly addressed from @ErikSchierboom to @iHiD, but as @tehsphinx points out, we also want to get it merged ASAP. So lots of trigger-ready people and no clear direction why it's not merged yet.

It'd be neat if we could just say "code owners approve and merge", but then I might need to approve my own pull requests. If that's where we're at, we will probably need to document some more formats. If not, we'll probably need to create some pull request policies. We have some ad-hoc rules on the Haskell track (exercism/haskell#752) for comparison. For example, a convention of writing under which circumstances something can be merged, provided either by the author of the pull request or the reviewer (conditional approval). Because the criteria may change after the original pull request message is made, it'd be nice to have a policy about when to merge/squash/rebase commits might be warranted.

[ErikSchierboom]

It'd be neat if we could just say "code owners approve and merge", but then I might need to approve my own pull requests

Maybe a little variation on this? If no single person was explicitly assigned, code owners should approve and merge. However, if I as a PR submitter want a specific person to review and merge the PR, I then request a review from that person (as opposed to the code owners).

Does this make sense?

[tehsphinx]

I Have been merging things last night that weren't really in my domain just to avoid merge conflicts as much as possible while all the maintainers start adding their data to the maintainer files.

Generally I intend to stick to merging only Go-related stuff. Approving PRs I think is something everyone is welcome to do.

[SleeplessByte]

I want to have a say in all things javascript and typescript and in general I think current maintainers should be reviewing prospect maintainer contents. I think that the other stuff: "typos etc" can be merged by anyone who has triple-checked them.

[iHiD]

@sshine Thanks for this. A few thoughts.

1) I imagine there will be an initial flurry on improving docs, but then I think most PRs will touch one languages/xxx. Some will also touch reference/, where clashes may emerge, but I imagine this will be rarer. So I hope merge-conflicts are rarer than might be expected right now. 2) I'm totally fine with tracks making their own rules about this, as long as there is a review step in that. So if JS wants to have a rule where a senior maintainer approves every PR before it's merged (which sounds sensible to me) then that's fine. But another track might not have the firepower for that, and are more willing to immediately move to something more akin to egalitarian approach. 3) At least at the beginning, I would rather @ErikSchierboom is assigned often to provide guidance. I would generally rather I wasn't assigned unless something needs a co-founder-level opinion, because having me as a blocker on things slows it down for everyone. 4) If senior maintainers (e.g. the three of you) want to just go round being helpful, that's great, but in many ways it's much better to give the newer maintainers the opportunity for cutting their teeth reviewing PRs etc.

One final thought, there are maintainers working on multiple tracks. It is important that if not consistency across tracks, there is documentation that those maintainers can refer to. So I would like us to have a Guide To Contributing (CONTRIBUTING.md?) which lists the global rules (e.g. everything needs a review) but also points to a common location in the langauges/xxx sections where track-specific rules might apply.

[iHiD]

Just looking at some of these PRs, we also need PRs that don't touch 100 files across repos. They get merged too quickly and there are errors in them. e.g. https://github.com/exercism/v3/pull/90

If big changes need to happen across the repo, these should be pre-coordinated with Erik I think.

So that needs to go into a policy too.

[SleeplessByte]

One final thought, there are maintainers working on multiple tracks. It is important that if not consistency across tracks, there is documentation that those maintainers can refer to. So I would like us to have a Guide To Contributing (CONTRIBUTING.md?) which lists the global rules (e.g. everything needs a review) but also points to a common location in the langauges/xxx sections where track-specific rules might apply.

Yes please.

[angelikatyborska]

I believe this issue becomes obsolete after we explode the v3 repo back into track repos.