From a79b1e7378c420c8de23b7de83700382b6fbfcf0 Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Thu, 4 Jun 2026 11:55:01 +0100 Subject: [PATCH 01/11] OPENR-99: Refactor and create new articles for making and reviewing a PR --- source/Contact.rst | 8 -- .../How-To-Guides/Core-maintainer-guide.rst | 19 ---- source/The-ROS2-Project/Contributing.rst | 10 +- .../Contributing/Contributing-to-code.rst | 8 ++ .../Contributing-to-code/Making-a-PR.rst | 103 ++++++++++++++++++ .../Contributing-to-code/Reviewing-a-PR.rst | 74 +++++++++++++ .../Contributing/Developer-Guide.rst | 62 +---------- 7 files changed, 189 insertions(+), 95 deletions(-) create mode 100644 source/The-ROS2-Project/Contributing/Contributing-to-code.rst create mode 100644 source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst create mode 100644 source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst diff --git a/source/Contact.rst b/source/Contact.rst index e8d72b9464a..1e3acca25fa 100644 --- a/source/Contact.rst +++ b/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 `__. -See the :doc:`Contributing ` page for more details and etiquette on how to contribute. - .. _Using ROS Discourse: Discussion diff --git a/source/How-To-Guides/Core-maintainer-guide.rst b/source/How-To-Guides/Core-maintainer-guide.rst index c740e2c58fa..bb0ec419c28 100644 --- a/source/How-To-Guides/Core-maintainer-guide.rst +++ b/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 ---------------------- diff --git a/source/The-ROS2-Project/Contributing.rst b/source/The-ROS2-Project/Contributing.rst index e775d07cc8f..b7dc02d29e9 100644 --- a/source/The-ROS2-Project/Contributing.rst +++ b/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 `__ 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 `__. - -We ask all contributors to follow the practices explained in :doc:`the developer guide `. - -Please be sure to :ref:`run tests ` for your code changes because most packages have tests that check that the code complies with our style guidelines. - Becoming a core maintainer ^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code.rst new file mode 100644 index 00000000000..a380d548de1 --- /dev/null +++ b/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 diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst new file mode 100644 index 00000000000..0e92a0141dc --- /dev/null +++ b/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 `__. + +For more information about contribution etiquette, see :doc:`Contributing <../../Contributing>`. + +Prerequisites +------------- + +#. `Create a fork `__ 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 `__. +#. 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 `. + * Make sure you've :ref:`run the tests ` 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 `__. + + For more information about each of the available pull request methods, see `the GitHub documentation `__. + +#. 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 `, 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 `__, 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. + +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. diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst new file mode 100644 index 00000000000..3b8a14677a9 --- /dev/null +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -0,0 +1,74 @@ +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 `__. + +Steps +----- + +1 Preparing for review +^^^^^^^^^^^^^^^^^^^^^^ + +* :ref:`Any developer ` is welcome to review a pull request. +* 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 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 `__), 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 `__). + +* :ref:`Any developer ` 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 ` 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. diff --git a/source/The-ROS2-Project/Contributing/Developer-Guide.rst b/source/The-ROS2-Project/Contributing/Developer-Guide.rst index a35cef0b5ea..ced3e7cd71a 100644 --- a/source/The-ROS2-Project/Contributing/Developer-Guide.rst +++ b/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 ------------------ @@ -153,8 +155,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 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -324,44 +324,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 `__. - -* 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 `` 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 `__ 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 ^^^^^^^^^^^^^^^^^^ @@ -715,7 +677,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, go through the Pull requests section for best practices. * For each repo to be modified: @@ -732,24 +694,6 @@ Before starting, go through the `Pull requests`_ section for best practices. See this `doc `__ 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 `__ 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 ----------------------- From fa246ac44602e12e0b1e3ceb0c1ff7870fe7ca95 Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Wed, 10 Jun 2026 15:54:22 +0100 Subject: [PATCH 02/11] Update source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst Co-authored-by: Katherine Scott Signed-off-by: Keith Kirkwood --- .../Contributing/Contributing-to-code/Making-a-PR.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst index 0e92a0141dc..ef6dc34e3c5 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst @@ -77,7 +77,7 @@ Use the following guidelines to prepare your 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)``. + * **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. From 800ca08c6e825ec183d448550231701e72e026fe Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Wed, 10 Jun 2026 16:25:14 +0100 Subject: [PATCH 03/11] OPENR-100: Updates from review in PR --- source/How-To-Guides/Core-maintainer-guide.rst | 6 ++++-- .../Contributing/Contributing-to-code/Making-a-PR.rst | 2 +- .../Contributing/Contributing-to-code/Reviewing-a-PR.rst | 3 +++ source/The-ROS2-Project/Contributing/Developer-Guide.rst | 2 +- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/source/How-To-Guides/Core-maintainer-guide.rst b/source/How-To-Guides/Core-maintainer-guide.rst index bb0ec419c28..8eaddbbbe14 100644 --- a/source/How-To-Guides/Core-maintainer-guide.rst +++ b/source/How-To-Guides/Core-maintainer-guide.rst @@ -44,7 +44,7 @@ This can be accomplished by using the colcon arguments ``--packages-up-to``, ``- See the `colcon documentation `__ 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: @@ -52,7 +52,9 @@ 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 `__. It is highly recommended to check the nightlies after merging pull requests to ensure no regressions have occurred. diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst index ef6dc34e3c5..478a2101576 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst @@ -31,7 +31,7 @@ Prerequisites * 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 `. - * Make sure you've :ref:`run the tests ` for your code changes. + * Make sure you've :ref:`run the tests ` and the appropriate linter for your code changes. * If your pull request is for a documentation change: diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index 3b8a14677a9..ceb832e8071 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -30,6 +30,9 @@ Steps ^^^^^^^^^^^^^^^^^^^^^^ * :ref:`Any developer ` 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: diff --git a/source/The-ROS2-Project/Contributing/Developer-Guide.rst b/source/The-ROS2-Project/Contributing/Developer-Guide.rst index 02d81544700..ff81ca10854 100644 --- a/source/The-ROS2-Project/Contributing/Developer-Guide.rst +++ b/source/The-ROS2-Project/Contributing/Developer-Guide.rst @@ -685,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: From 1aa1138de54aeed9d8e0ca77257982d14296a3ff Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Wed, 10 Jun 2026 16:42:29 +0100 Subject: [PATCH 04/11] OPENR-99: Fix linter error of trailing whitespace --- .../Contributing/Contributing-to-code/Reviewing-a-PR.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index ceb832e8071..e0d66eea1b3 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -30,7 +30,7 @@ Steps ^^^^^^^^^^^^^^^^^^^^^^ * :ref:`Any developer ` 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. From 2ddb485ecdb4e125d9803be3202c1aa859e5ed60 Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Wed, 17 Jun 2026 12:17:58 +0100 Subject: [PATCH 05/11] Update source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst Co-authored-by: Christophe Bedard Signed-off-by: Keith Kirkwood --- .../Contributing/Contributing-to-code/Reviewing-a-PR.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index e0d66eea1b3..8a6f42dbc7a 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -59,7 +59,7 @@ Use the following guidelines to review the submitted pull request: * 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 `__), or suggest changes directly in the pull request. +You can comment on the pull request for the submitter, or suggest changes directly in the pull request (`see the GitHub documentation for guidance `__). 3 Approving and merging the pull request ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From ecceb001a15dcbe6df489a52bd45eea4317a70ba Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Wed, 17 Jun 2026 12:20:41 +0100 Subject: [PATCH 06/11] Update source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst Co-authored-by: Christophe Bedard Signed-off-by: Keith Kirkwood --- .../Contributing/Contributing-to-code/Reviewing-a-PR.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index 8a6f42dbc7a..d23194515c8 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -68,7 +68,7 @@ After you have reviewed the pull request and provided feedback, the submitter ma When you are satisfied with the changes and they are ready to be merged, approve the pull request (`see the GitHub documentation for guidance `__). -* :ref:`Any developer ` is welcome to approve a pull request when the changes are ready to be merged. +* :ref:`Any developer ` is welcome to review a pull request, even if it already has a review. * 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. From 175a7be715e932ca22a4c641236251449c774235 Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Wed, 17 Jun 2026 16:10:10 +0100 Subject: [PATCH 07/11] OPENR-100: More review updates to articles from community contributors --- .../Contributing/Contributing-to-code/Making-a-PR.rst | 10 +++++++--- .../Contributing-to-code/Reviewing-a-PR.rst | 3 ++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst index 478a2101576..923e9e53eec 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst @@ -24,7 +24,7 @@ Prerequisites ------------- #. `Create a fork `__ 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 `__. +#. Complete your code changes on a development branch taken from the **rolling** branch, in your fork of the `target ROS repository `__. #. Make sure your changes comply with ROS guidelines. * If your pull request is for a code change: @@ -62,7 +62,7 @@ Use the following guidelines to prepare your pull request: * 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. + * 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 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -82,6 +82,8 @@ Use the following guidelines to prepare your pull request: * **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 `__ 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 :doc:`review your changes `, including checking against the relevant guidelines. 3 Responding to review comments @@ -100,4 +102,6 @@ Aim to reply back to review comments within one week, so that you and the review 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. +When the core maintainer approves your pull request, they merge it to the target branch (usually **rolling**), and you receive a notification from GitHub. + +Your changes may also be backported to older distributions of ROS. diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index e0d66eea1b3..49c93d5121d 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -15,7 +15,8 @@ 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. +Any developer is welcome to review and approve a pull request. +Changes are ready to merge after they have been approved. Only a ROS core maintainer for the target repository can merge a pull request into that repository. Prerequisites From f3435bc4001f1eb5bc316e96f0ae349016bfb102 Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Fri, 19 Jun 2026 15:06:21 +0100 Subject: [PATCH 08/11] OPENR-100: Add information about types of comments --- .../Contributing-to-code/Reviewing-a-PR.rst | 47 ++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index c8503c0c556..e23dd8cbc46 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -60,7 +60,52 @@ Use the following guidelines to review the submitted pull request: * 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, or suggest changes directly in the pull request (`see the GitHub documentation for guidance `__). +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 `__). + +Use the following guidance to make sure your review comments are useful and actionable: + +* Start with high-level comments (for example, asking for refactoring), then move on to lower-level comments about specifics. +* To be helpful to the submitter of the pull request, consider providing the following types of comment: + + * **Positive feedback** — praise or positive reinforcement highlighting good work, for example: + + Nice work on handling edge cases here — the early return makes the logic much easier to follow. + + * **Questions** — asking for clarification to understand intent or decisions made in the changes, for example: + + Is there a reason we're using a custom sorting function here instead of ``localeCompare``? + Just want to make sure Im not missing a specific requirement. + + * **Suggestions** — an improvement that enhances code quality but is not required to merge the pull request, 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** — a concrete problem which must be fixed before merging the pull request, ideally with a suggested solution, 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 ``users`` to better reflect that it's a collection. + (Non-blocking; feel free to ignore if you prefer the current name.) + +* Be clear about what you expect to happen in response to each comment, including whether the comment blocks merging the pull request. +* Remember to include positive feedback and thanks for the work done by the submitter. 3 Approving and merging the pull request ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 4ab801078b919e4dad3f678a29cf0934d81ebcb7 Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Mon, 22 Jun 2026 13:55:19 +0100 Subject: [PATCH 09/11] OPENR-100: Updates to Reviewing a PR for clarity/brevity --- .../Contributing-to-code/Reviewing-a-PR.rst | 75 +++++++++---------- 1 file changed, 37 insertions(+), 38 deletions(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index e23dd8cbc46..f0816671816 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -45,67 +45,66 @@ Steps 2 Reviewing the pull request ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -Use the following guidelines to review the submitted 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, verify that the changes are consistent with the design. -* For code changes, ensure that the changes: + * 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. + * 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. + * 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 add review comments to the pull request for the submitter, or suggest changes directly in the pull request (`see the GitHub documentation for guidance `__). +#. Provide your review comments. -Use the following guidance to make sure your review comments are useful and actionable: + 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 `__). -* Start with high-level comments (for example, asking for refactoring), then move on to lower-level comments about specifics. -* To be helpful to the submitter of the pull request, consider providing the following types of comment: +#. Follow these guidelines to make sure your review comments are useful and actionable: - * **Positive feedback** — praise or positive reinforcement highlighting good work, for example: + * Start with high-level comments (for example, asking for refactoring), then move on to lower-level comments about specifics. + * Consider providing the following types of comment: - Nice work on handling edge cases here — the early return makes the logic much easier to follow. + * **Positive feedback** — for example: - * **Questions** — asking for clarification to understand intent or decisions made in the changes, for example: + ``Nice work on handling edge cases here — the early return makes the logic much easier to follow.`` - Is there a reason we're using a custom sorting function here instead of ``localeCompare``? - Just want to make sure Im not missing a specific requirement. + * **Questions** — for example: - * **Suggestions** — an improvement that enhances code quality but is not required to merge the pull request, 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?`` - You could simplify this loop using ``Array.map`` to make it more concise: + * **Suggestions** — for example: - .. code-block:: javascript + ``You could simplify this loop using Array.map to make it more concise:`` - const names = users.map(user => user.name) + .. code-block:: javascript - * **Issues** — a concrete problem which must be fixed before merging the pull request, ideally with a suggested solution, for example: + const names = users.map(user => user.name) - This function doesn't handle the case where ``response`` is null, which could cause a runtime error. - Add a guard clause: + * **Issues** — for example: - .. code-block:: javascript + ``This function doesn't handle the case where response is null, which could cause a runtime error — add a guard clause:`` - if (!response) { - return [...]; - } + .. code-block:: javascript - * **Housekeeping** — a change that isn't related to the main purpose of the pull request, but helps to keep the repository healthy, for example: + if (!response) { + return [...]; + } - Since this file is already being updated, could we also remove the unused ``formatDate`` import at the top? + * **Housekeeping** — a change that isn't related to the main purpose of the pull request, but helps to keep the repository healthy, for example: - * **Minor details** — small, nitpicking details such as improving style or readability, for example: + ``Since this file is already being updated, could we also remove the unused formatDate import at the top?`` - Minor naming suggestion; ``user_list`` could be ``users`` to better reflect that it's a collection. - (Non-blocking; feel free to ignore if you prefer the current name.) + * **Minor details** — small, nitpicking details such as improving style or readability, for example: -* Be clear about what you expect to happen in response to each comment, including whether the comment blocks merging the pull request. -* Remember to include positive feedback and thanks for the work done by the submitter. + ``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. + * Remember to include positive feedback and thanks for the work done by the submitter. 3 Approving and merging the pull request ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 4af098e36dcbc2d3acfadadcbcefc38b6f00671d Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Mon, 22 Jun 2026 16:33:40 +0100 Subject: [PATCH 10/11] OPENR-100: Updates to xrefs and related content --- .../Contributing-to-code/Making-a-PR.rst | 14 ++++++++++---- .../Contributing-to-code/Reviewing-a-PR.rst | 15 +++++++++++---- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst index 923e9e53eec..48867c5c056 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst @@ -5,7 +5,7 @@ Pull requests are used to contribute code and documentation changes to ROS proje 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** +**Area: contributing, community | Content-type: how-to | Experience: beginner, intermediate, expert** .. contents:: Table of Contents :depth: 2 @@ -84,15 +84,15 @@ Use the following guidelines to prepare your pull request: #. Select the `Allow edits by maintainers `__ 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 :doc:`review your changes `, including checking against the relevant guidelines. +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 receive a notification from GitHub. -You can view and discuss `review comments directly in GitHub `__, 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. +You can view and discuss review comments directly in GitHub (see `the GitHub documentation for assistance `__), 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 `__). 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. @@ -105,3 +105,9 @@ After you've actioned any feedback, your pull request must be approved by a core When the core maintainer approves your pull request, they merge it to the target branch (usually **rolling**), and you receive a notification from GitHub. Your changes may also be backported to older distributions of ROS. + +Related content +--------------- + +* :ref:`ROS development general principles ` +* :doc:`Reviewing-a-PR` \ No newline at end of file diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index f0816671816..f95df4bfcb4 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -5,7 +5,7 @@ All incoming code and documentation to ROS projects must be reviewed in a pull r 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** +**Area: contributing, community | Content-type: how-to | Experience: beginner, intermediate, expert** .. contents:: Table of Contents :depth: 2 @@ -30,7 +30,7 @@ Steps 1 Preparing for review ^^^^^^^^^^^^^^^^^^^^^^ -* :ref:`Any developer ` is welcome to review a pull request. +* Any developer is welcome to review a pull request. A pull request generally requires two reviews before it can be merged. @@ -40,7 +40,7 @@ Steps * 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 it is under review. +* When you begin reviewing a pull request, leave a comment to let others know it is under review. 2 Reviewing the pull request ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -113,10 +113,17 @@ After you have reviewed the pull request and provided feedback, the submitter ma When you are satisfied with the changes and they are ready to be merged, approve the pull request (`see the GitHub documentation for guidance `__). -* :ref:`Any developer ` is welcome to review a pull request, even if it already has a review. +* Any developer is welcome to review a pull request, even if it already has a review. * 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 ` 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 ` +* :doc:`Making-a-PR` +* `About pull request reviews `__ \ No newline at end of file From 76cfafffb82a7fedbd4c5b87e7b5dea6dd9ab0bb Mon Sep 17 00:00:00 2001 From: Keith Kirkwood Date: Mon, 22 Jun 2026 16:38:24 +0100 Subject: [PATCH 11/11] OPENR-100: Fix linter error at end of file --- .../Contributing/Contributing-to-code/Making-a-PR.rst | 2 +- .../Contributing/Contributing-to-code/Reviewing-a-PR.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst index 48867c5c056..39dadc5911e 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Making-a-PR.rst @@ -110,4 +110,4 @@ Related content --------------- * :ref:`ROS development general principles ` -* :doc:`Reviewing-a-PR` \ No newline at end of file +* :doc:`Reviewing-a-PR` diff --git a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst index f95df4bfcb4..a47dbf4d6c0 100644 --- a/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst +++ b/source/The-ROS2-Project/Contributing/Contributing-to-code/Reviewing-a-PR.rst @@ -126,4 +126,4 @@ Related content * :ref:`ROS development general principles ` * :doc:`Making-a-PR` -* `About pull request reviews `__ \ No newline at end of file +* `About pull request reviews `__