Share your changes

How to submit your code modifications for review?

The author of a pull request (PR) is responsible for its “life cycle”. He is responsible for contacting the various parties involved, following the review, responding to comments and correcting the code following review (you could also check dedicated page about code review).

In the case of a large PR, don’t hesitate to ask several reviewers to organize themselves, or even to carry out the review together, reviewers and author.

  1. Open a pull request
    Once your changes are ready, you have to request integration with the dev branch.

    If possible:

    • Make PR of logical and atomic units too (avoid mixing refactoring, new features and bug fix at the same time).
    • Add a description to PRs to explain what they do and why.
    • Help the reviewer by following advice given in mtlynch article.
    • Add tags area:<affected_area> to show which part of the application have been impacted. It can be done through the web interface.
  2. Take feedback into account
    Once your PR is open, other contributors can review your changes:

    • Any user can review your changes.
    • Your code has to be approved by a contributor familiar with the code.
    • All users are expected to take comments into account.
    • Comments tend to be written in an open and direct manner. The intent is to efficiently collaborate towards a solution we all agree on.
    • Once all discussions are resolved, a maintainer integrates the change.

For large PRs that are bound to evolve over time, keeping corrections during review in separate commits helps reviewers. In the case of multiple reviews by the same person, this can save full re-review (ask for help if necessary):

  1. If you believe somebody forgot to review / merge your change, please speak out, multiple times if needs be.

Suggested workflow

Here’s a suggested workflow.

It is necessary to communicate via instant messaging (Matrix, Slack, etc.) in order to guarantee the smooth flow of PR validation.

sequenceDiagram
  actor A as PR author
  actor R as Reviewer/Maintainer
  
  A->>R: Asks for a review, notifying some people
  R->>A: Answers yes or no
  
  loop Loop between author and reviewer
    R-->>+A: Comments, asks for changes
    A-->>R: Answers to comments or requested changes
    A-->>-R: Makes necessary changes in dedicated "fixups"
    R-->>A: Reviews changes
    R-->>A: Resolves requested changes/conversations if ok
  end
 
  A->>R: Rebase and apply fixups
  R->>A: Checks commits history
  R->>A: Approves or closes the PR
  Note left of R: & Merges if maintainer

Finally continue towards tests ‣