Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename variables of rcl types #193

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Rename variables of rcl types #193

merged 1 commit into from
Jun 10, 2022

Conversation

nnmm
Copy link
Contributor

@nnmm nnmm commented Jun 6, 2022

I find it confusing and not very descriptive that "handle" is used to refer to both any rcl binding struct, and the PublisherHandle etc. types.

This PR changes the variable names of rcl binding types to be more descriptive, e.g. use rcl_node as the variable name for an rcl_node_t. Afterwards, "handle" refers exclusively to the PublisherHandle etc. types.

What's maybe worth discussing: Is rcl_node also fine for a &mut rcl_node_t?

@nnmm nnmm requested a review from jhdcs June 6, 2022 13:25
@nnmm nnmm force-pushed the disambiguate_handle branch 2 times, most recently from be2153c to 7741dfc Compare June 6, 2022 20:58
@esteve esteve self-requested a review June 7, 2022 08:21
Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it the opposite, I like the uniformity that every wrapped rcl entity has the same field, rclpy and rcljava do that, for example. It's more confusing if every struct has a different name for what is essentially the same (a pointer to the underlying native structure).

What I'd prefer is instead getting rid of the handle structs (like PublisherHandle struct) and use rcl_publisher_t as the value for the handle field directly.

@jhdcs
Copy link
Collaborator

jhdcs commented Jun 7, 2022

I can (unfortunately) see it going both ways - to some people, it's probably easier to think about if the names are completely uniform (like what Esteve suggested). To other people, the explicit naming of the underlying type could be useful (which I think is what Nikolai is going for with this PR).

Personally, like Nikolai, I like the explicit naming of the underlying type. However, since this is admittedly my first time writing a client library, my preferences may not be fully in-line with what more experienced developers of client libraries are comfortable with. On the other hand, how many developers of client libraries are going to start moving over to work on ros2_rust from one of their other projects?

Perhaps we should bring this up at the next working group meeting, and get some community feedback? See which style is the most clear to the most people? Our ultimate goal should be readability to the maximum number of people, so as to encourage more participation in the creation/maintenance of the library. At least that's how I see it.

@nnmm
Copy link
Contributor Author

nnmm commented Jun 7, 2022

@esteve I had a brief look into one file in rclpy, and it seems they use my proposed naming convention too: https://github.com/ros2/rclpy/blob/2c8ffe098eefebeec5a49e906e2506da4e0db434/rclpy/src/rclpy/publisher.cpp#L49

@esteve
Copy link
Collaborator

esteve commented Jun 7, 2022

@nnmm you're right, I recall using handle in rclpy at some point, that must have changed when rclpy moved to C++ from C. Anyway, if you and @jhdcs agree on the same naming conventions, that's settled 🙂

One thing, is that I'd like the *Handle structs go away, they don't seem to be necessary any more.

@nnmm
Copy link
Contributor Author

nnmm commented Jun 7, 2022

@esteve Ok, then I'll implement Drop on Publisher and Subscription.

@esteve
Copy link
Collaborator

esteve commented Jun 7, 2022

@nnmm thanks! That change can be done in a separate PR if that's more convenient.

@nnmm
Copy link
Contributor Author

nnmm commented Jun 7, 2022

Yes, let's do that.

esteve
esteve previously approved these changes Jun 9, 2022
@esteve
Copy link
Collaborator

esteve commented Jun 9, 2022

@nnmm do you want to merge this first or you'd rather wait for the PR removing the *Handle structs?

@nnmm
Copy link
Contributor Author

nnmm commented Jun 9, 2022

@esteve This one first (but after 186), since it's already open.

@nnmm nnmm changed the title Disambiguate 'handle' as a variable name Rename variables of rcl types Jun 10, 2022
@nnmm
Copy link
Contributor Author

nnmm commented Jun 10, 2022

@esteve @jhdcs I had to rebase, could one of you re-approve?

@nnmm nnmm merged commit 5436d57 into main Jun 10, 2022
@delete-merged-branch delete-merged-branch bot deleted the disambiguate_handle branch June 10, 2022 20:32
@nnmm nnmm mentioned this pull request Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants