Add notes on unsafe comments, client library consistency, and reviewing to contributing.md #252
Conversation
docs/CONTRIBUTING.md
Outdated
| 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. |
There was a problem hiding this comment.
I'd add external consistency as well, API should follow the principle of least surprise, where there are no quirks in how similar functions are called and the types they return.
There was a problem hiding this comment.
Ah, by "external consistency" you mean consistency of the user-facing API within rclrs? Because I thought external consistency would refer to the part explained in lines 73 - 79, i.e. consistency between client libraries.
There was a problem hiding this comment.
I meant more in the sense of consistence in the rclrs API itself, over the years rclcpp and rclpy have their own quirks and sometimes the API is not entirely coherent. For example, rclcpp's create_subscription has the QoS parameter before the callback, where create_service has them swapped. It's a delicate line, because on the one hand we want to make it easy for people to port their applications and mimic the existing APIs, but if such APIs are not consistent, it may make writing new applications more confusing.
There was a problem hiding this comment.
In my opinion, if we have to make a decision to mimic rclcpp's API or rclpy's API, we should probably go for rclcpp - Rust is a bit closer to C++ than to Python, so (theoretically) it's more likely that a user of rclcpp would be using rclrs. At least that's my reasoning.
But yeah, ideally the APIs would be consistent across the board.
There was a problem hiding this comment.
Okay, sounds like we mean the same things with "internal consistency" and "external consistency". I rewrote it to say "consistency within rclrs" to avoid this dubious distinction.
docs/CONTRIBUTING.md
Outdated
| - 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). |
There was a problem hiding this comment.
I think the addition of Chesterton's fence here might be harmful, given that in other parts of this document it's advised to understand the rationale for a given design in rclcpp in order to be able to not port features 1:1
There was a problem hiding this comment.
Hmm, yes you could read it as "be wary of deviating from the design of other client libraries", but I meant it more like "if you find the design of rclcpp and rclpy illogical, that's probably a signal that you don't understand the problem they are solving there".
But I see that it might be misleading, so I'm removing it.
docs/CONTRIBUTING.md
Outdated
| 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. |
There was a problem hiding this comment.
Ah, by "external consistency" you mean consistency of the user-facing API within rclrs? Because I thought external consistency would refer to the part explained in lines 73 - 79, i.e. consistency between client libraries.
docs/CONTRIBUTING.md
Outdated
| - 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). |
There was a problem hiding this comment.
Hmm, yes you could read it as "be wary of deviating from the design of other client libraries", but I meant it more like "if you find the design of rclcpp and rclpy illogical, that's probably a signal that you don't understand the problem they are solving there".
But I see that it might be misleading, so I'm removing it.
|
Regarding contributions, something that personally makes reviews more difficult for me is force-pushes and squashing while the PR is being reviewed, because I can't point to a commit to check if a particular feedback has been addressed. Could you add a section about that? We squash commits when we merge a PR, so there's not much change in how merge notes look like. |
Added. I think all comments should be addressed now. |
|
@esteve I assume all your comments have been addressed and this can be merged, yes? |
Motivation: This documents several guidelines that have so far been implicit. I hope they represent a consensus opinion :)