From 7e279123f31f58d576096bb226dc9037802d91e4 Mon Sep 17 00:00:00 2001 From: nnmm Date: Tue, 17 May 2022 11:14:36 +0200 Subject: [PATCH 1/6] Add section on consistency with rclcpp/rclpy --- docs/CONTRIBUTING.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 770bfd8c9..bde8bd0c4 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -61,6 +61,25 @@ Do not cast references to raw pointers where it's not needed. Since references c Generally, a `rcl` type should have a `Drop` impl that calls its `fini()` function. However, there are some cases where this is not possible, e.g. when the type is part of another `rcl` type that owns and finalizes it, or when the `fini()` function takes extra arguments. In these cases, it needs to be decided individually if and where to call `fini()`, and care must be taken to not return early from a function with `?` before the relevant `fini()` functions have been called. + +## Consistency with major ROS 2 client libraries +Consistency with the two major ROS 2 client libraries, `rclcpp` and `rclpy`, is a goal of this project – and in particular `rclcpp`. + +That means that +- Roughly the same concepts should exist in all client libraries, e.g. services, parameters, etc. +- Types, functions, argument names etc. should have the same or similar names as their corresponding items in `rclcpp`/`rclpy` where possible +- Code should be structured into similar packages by default, e.g. `rosidl_generator_`, `rcl` etc. +- Features should be built on top of `rcl` by default, instead of re-implementing them from scratch + +This does _not_ mean that +- No variability between languages is allowed. What is easy to do, and what is considered idiomatic, is quite different between languages, and thus ROS 2 allows for considerable variability between client languages. +- Features should always be ported 1:1. Do not rely on the assumption that the best implementation for `rclrs` is whatever `rclcpp` does, but understand the motivation and status of the implementation in the other client libraries. For instance, sometimes a design is implemented incompletely by a client library, the feature is later found to have problems (e.g. the deadlocking of sync client calls in Python), or the client library is constrained by backwards compatibility. +- Internal consistency does not matter. It does. +- No deviations from the above guidelines are possible. + +In summary, understand the context of the features that you port, in order to avoid [_cargo-culting_](https://en.wikipedia.org/wiki/Cargo_cult_programming), and to be mindful of [Chesterton's fence](https://en.wiktionary.org/wiki/Chesterton%27s_fence). + + ## License Any contribution that you make to this repository will be under the Apache 2 License, as dictated by that From f2b0f8c9364eef565e87f4a1cefaeaab5662a0ed Mon Sep 17 00:00:00 2001 From: nnmm Date: Sat, 10 Sep 2022 15:40:17 +0200 Subject: [PATCH 2/6] Recommend https://github.com/mre/idiomatic-rust --- docs/CONTRIBUTING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index bde8bd0c4..151e42da9 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -9,6 +9,8 @@ There are also occasional ROS 2 Rust WG meetings that are announced on the [ROS ## Coding guidelines This section is not comprehensive, and conventions from the broader Rust community should be followed. +If you're looking for resources to learn idiomatic Rust, a good starting point is https://github.com/mre/idiomatic-rust. + Use `cargo clippy` to check that code is idiomatic. ### Documentation From 4849e95d265c42ea1181d34c5693164a32aaef03 Mon Sep 17 00:00:00 2001 From: nnmm Date: Sat, 10 Sep 2022 15:54:56 +0200 Subject: [PATCH 3/6] Expand on safety comments --- docs/CONTRIBUTING.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 151e42da9..9f13c7605 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -22,7 +22,12 @@ When linking to documents from the ROS 2 ecosystem, always use the a link that r ### Unsafe code Keep `unsafe` blocks as small as possible. -Annotate every `unsafe` block with a `// SAFETY:` comment explaining why its safety preconditions are met. + +Annotate every `unsafe` block with a `// SAFETY:` comment explaining why it is safe. For function calls, that should cover the function's preconditions, if any, and any other interesting considerations. For instance, if you'd call [`rcl_context_get_init_options()`](https://github.com/ros2/rcl/blob/4b125b1af0e2e2c8c7dd0c8e18b5a8d36709058c/rcl/include/rcl/context.h#L217), the comment should explain whether the pointer that is returned by the function needs to be freed by the caller later, which hinges on whether it's a deep copy of the init options stored inside the context. If it's not a deep copy, it should be explained how it is guaranteed that the init options pointer does not outlive the context (e.g. because it is immediately converted to an owned type). + +In many cases, the only precondition is that the function arguments are valid pointers, which is trivially true if they are created from references. In such cases, a very brief safety comment such as "pointers are trivially valid" is sufficient. + +Inside `unsafe` functions, you can use `/* unsafe */` comments as a substitute for the `unsafe` keyword. ### Formatting Use `cargo fmt` to format code. From 9961e3e98553b0607552b3422595030856b3f3bf Mon Sep 17 00:00:00 2001 From: nnmm Date: Sat, 10 Sep 2022 16:00:45 +0200 Subject: [PATCH 4/6] Add section on reviewing code --- docs/CONTRIBUTING.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 9f13c7605..bfcd5e0c8 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -87,6 +87,13 @@ This does _not_ mean that In summary, understand the context of the features that you port, in order to avoid [_cargo-culting_](https://en.wikipedia.org/wiki/Cargo_cult_programming), and to be mindful of [Chesterton's fence](https://en.wiktionary.org/wiki/Chesterton%27s_fence). +## Reviewing code + +When reviewing pull requests, try to understand and give feedback on the high-level design first, before commenting on coding style, safety comments and such. + +Use the "changes requested" status sparingly, and instead consider simply leaving comments. This enables other reviewers to approve the pull requests when they see that all requested changes have been made. Conversely, if you are a reviewer and see that someone else has left comments requesting changes, it's expected that you ensure that all of them have been addressed before you approve. + + ## License Any contribution that you make to this repository will be under the Apache 2 License, as dictated by that From 34f26381c371fc745339a5d881a018eefafb8c61 Mon Sep 17 00:00:00 2001 From: nnmm Date: Wed, 28 Sep 2022 20:06:12 +0200 Subject: [PATCH 5/6] Implement review suggestions --- docs/CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index bfcd5e0c8..3512129f0 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -79,12 +79,12 @@ That means that - Features should be built on top of `rcl` by default, instead of re-implementing them from scratch This does _not_ mean that -- No variability between languages is allowed. What is easy to do, and what is considered idiomatic, is quite different between languages, and thus ROS 2 allows for considerable variability between client languages. +- No variability between languages is allowed. What is easy to do, and what is considered idiomatic, is quite different between languages, and thus ROS 2 allows for considerable variability between client languages. Excessively complex designs in C++ or Python may be caused by limitations of such languages and it's best to strive for clean, simple functionality in Rust, possibly leveraging zero-cost abstractions that are unique to the language. - Features should always be ported 1:1. Do not rely on the assumption that the best implementation for `rclrs` is whatever `rclcpp` does, but understand the motivation and status of the implementation in the other client libraries. For instance, sometimes a design is implemented incompletely by a client library, the feature is later found to have problems (e.g. the deadlocking of sync client calls in Python), or the client library is constrained by backwards compatibility. - Internal consistency does not matter. It does. - No deviations from the above guidelines are possible. -In summary, understand the context of the features that you port, in order to avoid [_cargo-culting_](https://en.wikipedia.org/wiki/Cargo_cult_programming), and to be mindful of [Chesterton's fence](https://en.wiktionary.org/wiki/Chesterton%27s_fence). +In summary, understand the context of the features that you port, in order to avoid [_cargo-culting_](https://en.wikipedia.org/wiki/Cargo_cult_programming). ## Reviewing code From a67cfb96c12cbeaaa75ce2759ebef5f0a55572b9 Mon Sep 17 00:00:00 2001 From: Nikolai Morin Date: Fri, 30 Sep 2022 15:51:55 +0200 Subject: [PATCH 6/6] Add section about squashing and amending --- docs/CONTRIBUTING.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 3512129f0..54dc00a55 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -81,14 +81,19 @@ That means that This does _not_ mean that - No variability between languages is allowed. What is easy to do, and what is considered idiomatic, is quite different between languages, and thus ROS 2 allows for considerable variability between client languages. Excessively complex designs in C++ or Python may be caused by limitations of such languages and it's best to strive for clean, simple functionality in Rust, possibly leveraging zero-cost abstractions that are unique to the language. - Features should always be ported 1:1. Do not rely on the assumption that the best implementation for `rclrs` is whatever `rclcpp` does, but understand the motivation and status of the implementation in the other client libraries. For instance, sometimes a design is implemented incompletely by a client library, the feature is later found to have problems (e.g. the deadlocking of sync client calls in Python), or the client library is constrained by backwards compatibility. -- Internal consistency does not matter. It does. +- Consistency within `rclrs` does not matter. It does. - No deviations from the above guidelines are possible. In summary, understand the context of the features that you port, in order to avoid [_cargo-culting_](https://en.wikipedia.org/wiki/Cargo_cult_programming). -## Reviewing code +## Squashing and amending commits +As soon as a PR is in review, it is generally preferable to not squash and amend commits anymore without the agreement of the reviewer. +The reason is that changes – especially large ones – are easier to follow when they are added in new commits, and that those commits can be referenced in review discussions. +When the PR is merged, all commits in the PR are squashed anyway. + +## Reviewing code When reviewing pull requests, try to understand and give feedback on the high-level design first, before commenting on coding style, safety comments and such. Use the "changes requested" status sparingly, and instead consider simply leaving comments. This enables other reviewers to approve the pull requests when they see that all requested changes have been made. Conversely, if you are a reviewer and see that someone else has left comments requesting changes, it's expected that you ensure that all of them have been addressed before you approve.