diff --git a/docs/devel/contributing/code-review.rst b/docs/devel/contributing/code-review.rst index 9d85ba7957df84b30cc669d2804d7f22c8d434e0..8d438494f57cf39c07a7366bdf04abe9a5a57bc0 100644 --- a/docs/devel/contributing/code-review.rst +++ b/docs/devel/contributing/code-review.rst @@ -1,39 +1,92 @@ .. _code-review: -Code Review +Code review =========== This page documents code review practices used for Software Heritage development. +`GitLab has many useful features +<https://docs.gitlab.com/ee/user/project/merge_requests/reviews/>`_ to ease +reviewing the changes, communicating around questions or comments, and making +suggestions. Learning how to use them will help you and the team. + Guidelines ---------- -Please adhere to the following guidelines to perform and obtain code reviews -(CRs) in the context of Software Heritage development: - -1. **CRs are strongly recommended** for any non-trivial code change, - but not mandatory (nor enforced at the VCS level). -2. The CR :ref:`workflow <patch-submission>` is implemented using - Phabricator/Differential. -3. Explicitly **suggest reviewer(s)** when submitting new CR requests: - either the most knowledgeable person(s) for the target code or the general - `reviewers <https://forge.softwareheritage.org/project/view/50/>`_ - (which is the `default <https://forge.softwareheritage.org/H18>`_). -4. **Review anything you want**: no matter the suggested reviewer(s), - feel free to review any outstanding CR. -5. **One LGTM is enough**: feel free to approve any outstanding CR. -6. **Review every day**: CRs should be timely as fellow developers - will wait for them. - To make CRs sustainable each developer should strive to dedicate - a fixed minimum amount of CR time every (work) day. - -For more detailed suggestions (and much more) on the motivational -and practical aspects of code reviews see Good reads below. - -Good reads ----------- +Please adhere to the following guidelines in the context of Software Heritage +development. + +When submitting changes: + +- **Reviews are strongly recommended** for any non-trivial code change, + but not mandatory (nor enforced at Git level). +- The :ref:`code submission workflow <patch-submission>` is implemented using + merge requests sent to our GitLab instance. +- The `assignee + <https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#assignee>`_ + in the context of merge request is the person supposed to drive it to + completion. **In almost all cases, you should assign yourself to merge requests you + create.** +- If your changes are meant to address a specific issue, you should `prefix the + branch name with the issue number + <https://docs.gitlab.com/ee/user/project/repository/branches/index.html#naming>`_. +- Feel free to explicitly **mention one or more specific + reviewer** from people most knowledgeable with the target code. You can also + assign a single person [#multiple-reviewers]_ as reviewer using the relevant + field. It will prevent the merge request from going unnoticed. +- **One approval is enough** before merging a request. + +As a team member: + +- **Review any merge requests you want**: no matter the suggested reviewers, + feel free to review any pending merge requests. +- **Review every day**: reviews should be timely as fellow developers + will wait for them. To make the process sustainable each developer should + strive to dedicate a fixed minimum amount of review time every workday. + +.. [#multiple-reviewers] Assigning multiple reviewers are only supported in + paid editions of GitLab. As we currently use the free *Community Edition*, + mentions are the only way to get the attention of multiple people at once. + +Learning about pending reviews +------------------------------ + +As we aim to review merge requests in a timely manner, here are several ways to +know about merge requests waiting for input. + +Notifications +^^^^^^^^^^^^^ + +To be sure to receive notifications of new merge requests: + +1. Open the `notification controls + <https://gitlab.softwareheritage.org/-/profile/notifications>`_ for your + GitLab account. +2. Locate the *Platform* group. +3. Select *Custom* in the list of notification levels. +4. Click on the bell icon. +5. Make sure *New merge request* is ticked. You probably also want to select at + least *New issue*, *Failed pipeline*, and *Fixed pipeline*. +6. Click *Ok*. +7. Repeat the operation for other groups of interest. + +List pending merge requests +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Sadly, notifications can easily be missed and GitLab by itself does not provide +a view with all merge requests waiting for action across multiple projects. To +have a better view of pending merge requests, you should consider using: + +- `GitLab Notify Extension + <https://github.com/Mikescops/gitlab-notify-extension>`_ available for Chrome + and Firefox. It can list all merge requests you are assigned to and all merge + requests you created. + +- `gitlab-revq <https://gitlab.softwareheritage.org/vlorentz/gitlab-revq/>`_, a + command-line tool listing all actionable pending requests. -Good reads on various angles of code review: +Recommended readings +-------------------- * `Best practices (Palantir) <https://medium.com/palantir/code-review-best-practices-19e02780015f>`_ ↠comprehensive and recommended read, especially if you're short on time * `Best practices (Thoughtbot) <https://github.com/thoughtbot/guides/tree/master/code-review>`_ diff --git a/docs/devel/contributing/gitlab.rst b/docs/devel/contributing/gitlab.rst index c447ab3866b3575e038211cec43330de72cc172d..e1f580941d712cd8e7671ece5fde86a688df4707 100644 --- a/docs/devel/contributing/gitlab.rst +++ b/docs/devel/contributing/gitlab.rst @@ -32,6 +32,8 @@ documentation. Please follow the steps below to setup your account. - Sign the Software Heritage Contributor License Agreement. Please contact us to know more about this. +.. _patch-submission: + Development workflow -------------------- diff --git a/docs/devel/contributing/phabricator.rst b/docs/devel/contributing/phabricator.rst index 40b26afe4253bac5f583a7740a1c8fac5e57c03d..4f434be5fbfe74b4c475c21505e32ea6c043781f 100644 --- a/docs/devel/contributing/phabricator.rst +++ b/docs/devel/contributing/phabricator.rst @@ -15,8 +15,6 @@ https://gitlab.softwareheritage.org/ The content below is no longer relevant and will be updated soon. -.. _patch-submission: - Submitting patches ==================