3di | Making and reviewing a pull request

source/Contact.rst

@@ -78,14 +78,6 @@ As appropriate, also include your:
 * Bag files and code samples that can reproduce the problem
 * Gifs or movies to demonstrate the problem
 
-
-Pull requests
--------------
-
-When you feel comfortable enough to suggest a specific change directly to the code, you can submit a pull request.
-Pull requests are welcome for any of `the ros2 repositories <https://github.com/ros2>`__.
-See the :doc:`Contributing <The-ROS2-Project/Contributing>` page for more details and etiquette on how to contribute.
-
 .. _Using ROS Discourse:
 
 Discussion

source/How-To-Guides/Core-maintainer-guide.rst

@@ -12,25 +12,6 @@ This guide gives some information about the responsibilities of a ROS 2 core pac
 .. contents:: Table of Contents
    :local:
 
-Reviews
--------
-
-All incoming code to ROS 2 core repositories must be reviewed.
-The review is looking for:
-
-* Suitability in the package
-* Correct code
-* Conforms to developer guidelines:
-
-    * :doc:`Developer Guide <../The-ROS2-Project/Contributing/Developer-Guide>`
-    * :doc:`Code Style Guide <../The-ROS2-Project/Contributing/Code-Style-Language-Versions>`
-
-* Adds tests for the bug/feature
-* Adds documentation for new features
-* Clean Continuous Integration run
-* Targets default branch (usually "rolling")
-* Has at least one approval from a maintainer that is not the author
-
 Continuous Integration
 ----------------------
 
@@ -63,15 +44,17 @@ This can be accomplished by using the colcon arguments ``--packages-up-to``, ``-
 See the `colcon documentation <https://colcon.readthedocs.io/en/released/user/how-to.html#build-only-a-single-package-or-selected-packages>`__ for more examples on the flags that can be used.
 Further documentation on how to use the CI machinery is available at https://github.com/ros2/ci/blob/master/CI_BUILDERS.md.
 
-Merging Pull Requests
+Merging pull requests
 ---------------------
 
 A pull request can be merged if all of the following are true:
 
 * The DCO bot reports a passing result
 * The PR build reports a passing result
 * The CI build reports a passing result on all platforms
-* The code has been reviewed and approved by at least one maintainer
+* The PR has been reviewed and approved by at least one maintainer
+
+For more information about what happens when a PR is reviewed, see :doc:`../The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR`.
 
 After a PR is merged, it will automatically get built with the next `nightlies <https://ci.ros2.org/view/nightly>`__.
 It is highly recommended to check the nightlies after merging pull requests to ensure no regressions have occurred.

source/The-ROS2-Project/Contributing.rst

@@ -69,6 +69,7 @@ Development Guides
    Contributing/Quality-Guide
    Contributing/Build-Farms
    Contributing/Windows-Tips-and-Tricks
+   Contributing/Contributing-to-code
    Contributing/Contributing-To-ROS-2-Documentation
 
 What to work on
@@ -85,15 +86,6 @@ If you have some code to contribute that fixes a bug or improves documentation,
 For larger changes, it is a good idea to discuss the proposal `on the ROS 2 forum <https://discourse.openrobotics.org/c/ros/111>`__ before you start to work on it so that you can identify if someone else is already working on something similar.
 If your proposal involves changes to the APIs, it is especially recommended that you discuss the approach before starting work.
 
-Submitting your code changes
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-Code contributions should be made via pull requests to `the appropriate ros2 repositories <https://github.com/ros2>`__.
-
-We ask all contributors to follow the practices explained in :doc:`the developer guide <Contributing/Developer-Guide>`.
-
-Please be sure to :ref:`run tests <colcon-run-the-tests>` for your code changes because most packages have tests that check that the code complies with our style guidelines.
-
 Becoming a core maintainer
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 

source/The-ROS2-Project/Contributing/Contributing-to-code.rst

@@ -0,0 +1,8 @@
+Contributing to code
+====================
+
+.. toctree::
+   :maxdepth: 1
+
+   Contributing-to-code/Making-a-PR
+   Contributing-to-code/Reviewing-a-PR

source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst

@@ -0,0 +1,103 @@
+Making a pull request (PR) — how-to
+===================================
+
+Pull requests are used to contribute code and documentation changes to ROS projects.
+This article explains how to prepare and create a pull request from your fork of a ROS repository.
+With this information, you'll be able to submit focused changes in a pull request, ready for review.
+
+**Area: community | Content-type: how-to | Experience: beginner, intermediate, expert**
+
+.. contents:: Table of Contents
+   :depth: 2
+   :local:
+
+Summary
+-------
+
+Pull requests (PRs) are proposals to merge your changes into a ROS project.
+Making a pull request allows you to collaborate with other ROS contributors, providing a space to discuss and review your code changes before a ROS maintainer merges them.
+Pull requests are welcome for any of `the ROS repositories <https://github.com/ros2>`__.
+
+For more information about contribution etiquette, see :doc:`Contributing <../../Contributing>`.
+
+Prerequisites
+-------------
+
+#. `Create a fork <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/fork-a-repo>`__ of the target ROS repository for your code changes.
+#. Complete your code changes on a development branch, in your fork of the `target ROS repository <https://github.com/ros2>`__.
+#. Make sure your changes comply with ROS guidelines.
+
+   * If your pull request is for a code change:
+
+     * Make sure you've followed the guidance in the :doc:`Developer guide <../Developer-Guide>`.
+     * Check that your code complies with the relevant section of the :doc:`Code style guide </The-ROS2-Project/Contributing/Code-Style-Language-Versions>`.
+     * Make sure you've :ref:`run the tests <colcon-run-the-tests>` and the appropriate linter for your code changes.
+
+   * If your pull request is for a documentation change:
+
+     * Make sure you've followed the guidance in :doc:`../Contributing-To-ROS-2-Documentation`.
+
+Steps
+-----
+
+1 Preparing the pull request
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Use the following guidelines to prepare your pull request:
+
+* **Scope and focus**
+   * Limit each pull request to a single, well defined change.
+   * Submit unrelated changes as separate pull requests.
+   * Keep patches small and avoid unnecessary or incidental changes.
+* **Commit history and squashing**
+   * Squash changes into a minimum number of clear, semantic commits to preserve a readable project history.
+   * Don't squash commits while a pull request is under review, as reviewers may not notice changes which can lead to confusion.
+   * You can create new commits while a pull request is under review.
+* **Draft pull requests**
+   * Use draft pull requests to request early feedback while work is in progress.
+   * Don't expect draft pull requests to be formally reviewed or merged until you have marked them as ready.
+   * If you want early feedback from a specific person on a draft pull request, mention them (using @) in the pull request description or in a comment.
+* **Mentions and references**
+   * If your changes are based on a design document, mention the package owners who reviewed the design in the pull request description.
+   * If your pull request depends on another pull request, clearly reference the dependency in the pull request description.
+   * If your changes are planned to be released with a specific version of ROS, include that version of ROS in the pull request description.
+* **Documenting your code changes**
+   * If your pull request is for code changes, ensure you propose any required documentation updates (including API documentation, feature documentation, and release notes) in another pull request.
+
+2 Submitting the pull request
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+#. Create a pull request from the branch containing your changes in your fork, to the **rolling** branch of the target ROS repository.
+   You can create your pull request using the GitHub CLI, GitHub Desktop, or using other methods.
+
+   For more information about creating a pull request from a fork, see `the GitHub documentation <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork>`__.
+
+   For more information about each of the available pull request methods, see `the GitHub documentation <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request>`__.
+
+#. Populate the pull request by completing the sections shown in the description template, including:
+
+   * **Description**: summarize your code changes and the related GitHub issue, highlighting any key points or areas of concern.
+   * **Issue**: include the ID of the GitHub issue fixed by your changes, in the format ``Fixes #(issue)``.
+     This ensures that this issue is automatically closed when the pull request is merged.
+   * **Generative AI**: if this pull request was generated using Generative AI, specify the model and version (for example, GitHub Copilot v3.2).
+   * **Additional information**: provide any context or details you think will be useful for understanding your changes.
+
+After you've submitted your pull request, other developers and contributors in the ROS community will :doc:`review your changes <Reviewing-a-PR>`, including checking against the relevant guidelines.
+
+3 Responding to review comments
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When another developer or contributor adds a review comment or suggestion to your pull request, you receive a notification from GitHub.
+
+You can view and discuss `review comments directly in GitHub <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/viewing-a-pull-request-review>`__, and add further commits to your branch to address them when needed.
+You can also directly `accept any suggested changes <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request>`__ in the pull request, which adds a new commit to your branch automatically.
+
+Discuss and iterate on your changes with this feedback, amending and updating your development branch with new commits as needed.
+Aim to reply back to review comments within one week, so that you and the reviewers do not lose the context of your changes.
+
+4 Merging the pull request
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+After you've actioned any feedback, your pull request must be approved by a core maintainer for the target ROS repository before it can be merged.
+
+When the core maintainer approves your pull request, they merge it to the target branch, and you receive a notification from GitHub.

source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst

@@ -0,0 +1,77 @@
+Reviewing a pull request (PR) — how-to
+======================================
+
+All incoming code and documentation to ROS projects must be reviewed in a pull request.
+This article explains how to prepare for and review a pull request submitted by a contributor.
+After reading this article, you'll be able to ensure changes in a pull request meet the required standards.
+
+**Area: community | Content-type: how-to | Experience: beginner, intermediate, expert**
+
+.. contents:: Table of Contents
+   :depth: 2
+   :local:
+
+Summary
+-------
+
+Reviewing a pull request (PR) from a contributor allows you to check that their changes meet the appropriate guidelines and standards.
+Any developer is welcome to review and approve a pull request after review, when the changes are ready to merge.
+Only a ROS core maintainer for the target repository can merge a pull request into that repository.
+
+Prerequisites
+-------------
+
+A code or documentation contributor has made a pull request to merge their changes into one of `the ROS repositories <https://github.com/ros2>`__.
+
+Steps
+-----
+
+1 Preparing for review
+^^^^^^^^^^^^^^^^^^^^^^
+
+* :ref:`Any developer <general-principles>` is welcome to review a pull request.
+
+  A pull request generally requires two reviews before it can be merged.
+
+* Treat reviewing a pull request as a collaborative activity involving the submitter and other developers, rather than a passive or one-way process.
+* As a reviewer:
+
+  * You can make small improvements to code or documentation in-place, such as fixing typos or addressing minor style issues.
+  * You should make a best effort attempt to comment on the pull request within one week of submission.
+
+* When you begin `reviewing a pull request <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews>`__, leave a comment to let others know it is under review.
+
+2 Reviewing the pull request
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Use the following guidelines to review the submitted pull request:
+
+* Confirm the code or documentation changes are appropriate for the repository.
+* Verify the code is correct and complete, and scoped to a single, well defined change.
+* Check that the pull request targets the default branch (usually ``rolling``).
+* If the changes are based on a design document, verify that the changes are consistent with the design.
+* For code changes, ensure that the changes:
+
+    * Follow the :doc:`Developer guide <../Developer-Guide>`.
+    * Follow the :doc:`Code style guide <../Code-Style-Language-Versions>`.
+    * Include tests for the new feature or bug fix.
+
+* For documentation changes, ensure the changes follow the :doc:`documentation guidance <../Contributing-To-ROS-2-Documentation>`.
+* Confirm that the Continuous Integration (CI) run for the pull request passes cleanly.
+
+You can comment on the pull request for the submitter (`see the GitHub documentation for guidance <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request>`__), or suggest changes directly in the pull request.
+
+3 Approving and merging the pull request
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+After you have reviewed the pull request and provided feedback, the submitter may continue the discussion or iterate on their changes, adding new commits to the PR.
+
+When you are satisfied with the changes and they are ready to be merged, approve the pull request (`see the GitHub documentation for guidance <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/approving-a-pull-request-with-required-reviews>`__).
+
+* :ref:`Any developer <general-principles>` is welcome to approve a pull request when the changes are ready to be merged.
+* A pull request must have at least one approval from a developer (other than the author) before it can be merged to the target branch.
+* Only a ROS core maintainer for the target repository can merge an approved pull request.
+
+  * See the :doc:`current ROS PMC constituents and committers </The-ROS2-Project/Governance>` for the list of people with merge permissions for the target repository.
+
+* If the pull request has any dependencies, ensure that dependent pull requests are merged in the correct order.

source/The-ROS2-Project/Contributing/Developer-Guide.rst

@@ -12,6 +12,8 @@ ROS 2 developer guide
 
 This page defines the practices and policies we employ when developing ROS 2.
 
+.. _general-principles:
+
 General Principles
 ------------------
 
@@ -161,8 +163,6 @@ Change control process
 
   * Packages may choose to increase this number.
 
-* Any required changes to documentation (API documentation, feature documentation, release notes, etc.) must be proposed before merging related changes.
-
 Guidelines for backporting PRs
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -332,44 +332,6 @@ For example: Development commits targeting ``foxy`` are made to the ``foxy`` bra
 
 ``main`` typically targets :doc:`Rolling <../../Releases/Release-Rolling-Ridley>` (and so, the next unreleased ROS distribution), though the maintainers may decide to develop and release from a ``rolling`` branch instead.
 
-Pull requests
-^^^^^^^^^^^^^
-
-* A pull request should only focus on one change.
-  Separate changes should go into separate pull requests.
-  See `GitHub's guide to writing the perfect pull request <https://github.com/blog/1943-how-to-write-the-perfect-pull-request>`__.
-
-* A patch should be minimal in size and avoid any kind of unnecessary changes.
-
-* A pull request must contain minimum number of meaningful commits.
-
-  * You can create new commits while the pull request is under review.
-
-* Before merging a pull request all changes should be squashed into a small number of semantic commits to keep the history clear.
-
-  * But avoid squashing commits while a pull request is under review.
-    Your reviewers might not notice that you made the change, thereby introducing potential for confusion.
-    Plus, you're going to squash before merging anyway; there's no benefit to doing it early.
-
-* Any developer is welcome to review and approve a pull request (see `General Principles`_).
-
-* When you are working on a change that is not ready for review or to be merged, use a draft pull request.
-  When that change is ready for review, move the pull request out of the draft state.
-  Note that if you want early feedback from specific people on a draft pull request, you can @ mention them in the pull request's description or in a comment on the pull request.
-
-* If your pull request depends on other pull requests, link to each depended on pull request by adding ``- Depends on <link>`` at the top of your pull request's description.
-  Doing so helps reviewers understand the context of the pull request.
-
-* When you start reviewing a pull request, comment on the pull request so that other developers know that you're reviewing it.
-
-* Pull-request review is not read-only, with the reviewer making comments and then waiting for the author to address them.
-  As a reviewer, feel free to make minor improvements (typos, style issues, etc.) in-place.
-  As the opener of a pull-request, if you are working in a fork, checking the box to `allow edits from upstream contributors <https://github.com/blog/2247-improving-collaboration-with-forks>`__ will assist with the aforementioned.
-  As a reviewer, also feel free to make more substantial improvements, but consider putting them in a separate branch (either mention the new branch in a comment, or open another pull request from the new branch to the original branch).
-
-* Only maintainers and committers can merge approved pull requests into the mainline.
-  See the :doc:`current ROS PMC constituents and committers <../Governance>` for the list of people with merge permissions.
-
 Library versioning
 ^^^^^^^^^^^^^^^^^^
 
@@ -723,7 +685,7 @@ maintainers of all impacted packages (as defined by ``package.xml`` maintainer f
 Implementation
 ~~~~~~~~~~~~~~
 
-Before starting, go through the `Pull requests`_ section for best practices.
+Before starting, look at :doc:`Contributing-to-code/Making-a-PR` for best practices in pull requests.
 
 * For each repo to be modified:
 
@@ -740,24 +702,6 @@ Before starting, go through the `Pull requests`_ section for best practices.
       See this `doc <https://help.github.com/articles/closing-issues-using-keywords/>`__ for more details.
     * Push the new commits.
 
-Code review
-~~~~~~~~~~~
-
-Once the change is ready for code review:
-
-* Open a pull request for each modified repository.
-
-  * Remember to follow `Pull requests`_ best practices.
-  * `GitHub <https://hub.github.com/>`__ can be used to create pull requests from the command-line.
-  * If the task is planned to be released with a specific version of ROS, this information should be included in each pull request.
-
-* Package owners who reviewed the design document should be mentioned in the pull request.
-* Code review SLO: although reviewing pull requests is best-effort,
-  it is helpful to have reviewers comment on pull requests within a week and
-  code authors to reply back to comments within a week, so there is no loss of context.
-* Iterate on feedback as usual, amend and update the development branch as needed.
-* Once the PR is approved, package maintainers will merge the changes in.
-
 
 Build Farm Introduction
 -----------------------