3di | Making and reviewing a PR (backport #6967)

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,6 +12,7 @@ This guide gives some information about the responsibilities of a ROS 2 core pac
 .. contents:: Table of Contents
    :local:
 
+<<<<<<< HEAD:source/How-To-Guides/Core-maintainer-guide.rst
 Reviews
 -------
 
@@ -31,6 +32,8 @@ The review is looking for:
 * Targets default branch (usually "rolling")
 * Has at least one approval from a maintainer that is not the author
 
+=======
+>>>>>>> a6fdb553 (3di | Making and reviewing a PR (#6967)):source/Developer-Tools/Build/Core-maintainer-guide.rst
 Continuous Integration
 ----------------------
 
@@ -63,15 +66,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,7 +69,12 @@ Development Guides
    Contributing/Quality-Guide
    Contributing/Build-Farms
    Contributing/Windows-Tips-and-Tricks
+<<<<<<< HEAD
    Contributing/Contributing-To-ROS-2-Documentation
+=======
+   Contributing/Contributing-to-code
+   Contributing/Contributing-to-documentation
+>>>>>>> a6fdb553 (3di | Making and reviewing a PR (#6967))
 
 What to work on
 ^^^^^^^^^^^^^^^
@@ -85,15 +90,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,114 @@
+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: contributing, community | Content-type: how-to | Experience: beginner, intermediate, expert**
+
+.. contents:: Table of Contents
+   :depth: 2
+   :local:
+
+Summary
+-------
+
+`Pull requests (PRs) <https://docs.github.com/en/pull-requests>`__ are proposals to merge your changes into a ROS repository.
+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 </The-ROS2-Project/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 taken from the **rolling** 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 </The-ROS2-Project/Contributing/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:`/The-ROS2-Project/Contributing/Contributing-to-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, such as a `REP <https://reps.openrobotics.org/>`__, mention other people involved in the design, such as those who reviewed the REP, in the pull request description.
+   * If your pull request depends on another pull request, clearly reference the dependency in the pull request description.
+     Ensure to mention the pull request ID using ``#`` notation.
+   * 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, try to make any relevant documentation updates (including API documentation, feature documentation, and release notes) in the same 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 the GitHub web interface.
+
+   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, linking to related GitHub issue(s) and PRs by ID, 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 the 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.
+
+#. Select the `Allow edits by maintainers <https://github.blog/news-insights/product-news/improving-collaboration-with-forks/>`__ checkbox, to help ROS maintainers make small changes directly when needed.
+
+After you've submitted your pull request, other developers and contributors in the ROS community will review your changes, 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 will receive a notification from GitHub.
+
+You can view and discuss review comments directly in GitHub (see `the GitHub documentation for assistance <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 in the pull request, which adds a new commit to your branch automatically (see `the GitHub documentation for how to accept suggested changes <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request>`__).
+
+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 :doc:`Committer for the target ROS repository </The-ROS2-Project/Governance>` before it can be merged.
+
+When the Committer approves your pull request, they will merge it to the target branch (usually **rolling**), and you will receive a notification from GitHub.
+
+Your changes may also be backported to older distributions of ROS.
+
+Related content
+---------------
+
+* :ref:`ROS development general principles <general-principles>`
+* :doc:`Reviewing-a-PR`

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

@@ -0,0 +1,129 @@
+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: contributing, 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.
+Anyone is welcome to review and approve a pull request.
+Changes are ready to merge after they have been approved.
+Only a :doc:`Committer </The-ROS2-Project/Governance>` for the target repository can merge a pull request into that repository, and they will not do so until it has been approved.
+
+Prerequisites
+-------------
+
+A code or documentation contributor has :doc:`made a pull request </The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR>` to merge their changes into one of `the ROS repositories <https://github.com/ros2>`__.
+
+Steps
+-----
+
+1 Preparing for review
+^^^^^^^^^^^^^^^^^^^^^^
+
+* Anyone 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, leave a comment to let others know you are performing a review.
+
+2 Reviewing the pull request
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+#. Review the pull request against the following guidelines:
+
+   * 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, such as a `REP <https://reps.openrobotics.org/>`__, 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 </The-ROS2-Project/Contributing/Contributing-to-documentation>`.
+   * Confirm that the Continuous Integration (CI) run for the pull request passes cleanly.
+
+#. Provide your review comments.
+
+   You can add review comments to the pull request for the submitter, or suggest changes directly in 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/commenting-on-a-pull-request>`__).
+
+#. Follow these guidelines to make sure your review comments are useful and actionable:
+
+   * Start with high-level comments (for example, asking for refactoring or design changes), then move on to lower-level comments about specifics.
+   * Consider providing the following types of comment:
+
+     * **Positive feedback** — for example:
+
+       ``Nice work on handling edge cases here — the early return makes the logic much easier to follow.``
+
+     * **Questions** — for example:
+
+       ``Just to make sure I'm not missing a requirement, is there a reason we're using a custom sorting function here instead of localeCompare?``
+
+     * **Suggestions** — for example:
+
+       ``You could simplify this loop using Array.map to make it more concise:``
+
+       .. code-block:: javascript
+
+         const names = users.map(user => user.name)
+
+     * **Issues** — for example:
+
+       ``This function doesn't handle the case where response is null, which could cause a runtime error — add a guard clause:``
+
+       .. code-block:: javascript
+
+         if (!response) {
+           return [...];
+         }
+
+     * **Housekeeping** — a change that isn't related to the main purpose of the pull request, but helps to keep the repository healthy, for example:
+
+       ``Since this file is already being updated, could we also remove the unused formatDate import at the top?``
+
+     * **Minor details** — small, nitpicking details such as improving style or readability, for example:
+
+       ``Minor naming suggestion; user_list could be named users to better reflect that it's a collection.``
+
+   * Be clear about what you expect to happen in response to each comment, including whether the comment blocks merging the pull request, and whether you consider your request optional or required.
+   * Remember to include positive feedback and thanks for the work done by the submitter, and always be constructive.
+
+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>`__).
+
+* Anyone is welcome to review a pull request, even if it already has a review.
+* A pull request must have at least one approval, and in most cases, two approvals, from a developer (other than the author) before it can be merged to the target branch.
+* Only a Committer for the target repository can merge an approved pull request.
+
+  * See the :doc:`current ROS 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.
+
+Related content
+---------------
+
+* :ref:`ROS development general principles <general-principles>`
+* :doc:`Making-a-PR`
+* `About pull request reviews <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews>`__