Skip to content

Reviewing code changes

Any changes to CERN projects should be reviewed. Reviews are part of continuous due diligence and as such a requirement for CERN open-source projects.

Projects should require reviews, at a minimum for the production code. A review by the author of the change is insufficient. This can be configured per GitHub repository or GitLab project.

Projects using CERN resources for their continuous integration must ensure that external non-trusted contributions are reviewed before continuous integration is run on the proposed change.

Authors appreciate quick review turnarounds, within hours not days: it will allow them to reason about what they wrote while they still remember why they wrote it.

Review scope

See due diligence for an introduction of what a review needs to cover as a minimum. Reviews should also cover these elements:

  • The new code must be tested, for instance by adding new tests for new features.
  • The change must not introduce test failures. Projects with significant test coverage but without test failures signal professionalism to potential contributors.
  • It should be clear that the code was written by the author, or if not it must be made clear where the code comes from and what license it has.
  • The change should not introduce offensive language in comments or complaints about someone else's code.

Expert reviews

"Code owners" for files or directories can be defined with Github and Gitlab. Merge / pull requests will then suggest the code owners of changed files as reviewers.

Reviews as contributions

Reviews should be seen as a way to contribute; other people becoming the code owner of a part of the project is an indication of a healthy project.

How to review

A bad review:

  • suggests out-of-scope changes;
  • considers only the code lines touched by the pull / merge request;
  • complains about the suggested changes;
  • doesn't suggest a way forward.

A good review on the other hand:

  • suggests what to improve and how, specifically within the existing changes of the pull / merge request;
  • takes the context into account, such as related, ongoing developments;
  • is motivating the creator of the pull / merge request to do further work.

That said, not all pull / merge requests should be accepted. Typical reasons include:

  • introducing a feature that is not seen as crucial ("feature bloat"), adding to the technical debt;
  • a change that conflicts with the project's planned evolution;
  • low-quality or irrelevant changes, for instance to appear as contributor;