Policy discussion: on pull requests, merging and reviews

[klenze]

There were some discussions in the phase zero meeting today regarding the feasibility of importing features from PRs currently awaiting acceptance.

There was an almost consensus (with @YanzhaoW vocally dissenting) that expecting the users to import stuff from multiple feature branches, then implement their own feature on top of that, and rebase to dev when these features are merged is not feasible for most users.

What I would propose instead is a three step process.

First comes a speedy review which focuses on the following points:

• Does the PR pass CI?
• Is it a clear improvement over status quo? (Code for a new detector is likely an improvement unless written horribly, code for a well established detector will have to meet higher standards.)
• Is the class design reasonable?
• Are their obvious bugs in it?
• Are the interfaces (e.g. method names) reasonable?

If the PR passes these questions, it is tentatively merged (with the source files clearly marked as preliminary) and a full review is scheduled. Anyone can use the new features, but should be aware that the API might still change.

Then comes the second in depth review.

• Is the implementation well written, robust and maintainable?
• Are there test cases?
• Are corner cases and failures handled in a reasonable way
• Does the code appear to be bug-free?

The original author will work with the reviewers to bring the implementation up to our standard. If the author can't or won't make changes, it is up to other people interested in the feature to fix things. If authors and reviewers can't agree, maintainers will decide.

If change requests are not addressed by anyone, the feature is removed from dev.

All preexisting code should also be scheduled for review. I am sure @YanzhaoW would have some comments on the task "template" files, for example.

--

Next proposal:

Furthermore, each class should have at least one sponsor. That is someone who needs the feature, understands the code (or at least can spend the time to understand it when required), will review relevant PRs and fix stuff when APIs change.

If there is no sponsor for a class it gets removed. If someone needs that feature again ten years down the line, let them copy it from the version history.