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

Naming of fields for Time message is inconsistent with rclcpp::Time #166

Open
YoshuaNava opened this issue May 2, 2024 · 3 comments
Open
Labels

Comments

@YoshuaNava
Copy link

I'm working with both builtin_interfaces/msg/Time and rclcpp::Time, and I found that the naming of the fields is inconsistent between them:

  • For rclcpp::Time fields are called seconds and nanoseconds
  • But for builtin_interfaces::msg::Time fields are called sec and nanosec.

Would it be possible for both time representations to use the same field names? If possible I would suggest following the convention from rclcpp::Time, which uses full yet succint names.

Thank you in advance! 🙏

Yoshua

@clalancette
Copy link
Contributor

I'm not a huge fan of making this change. While I agree it is annoying that they are inconsistent, neither is unclear. And to fix either of them, we'd have to break some code that uses it. So my view on this is that it is a "wontfix", but maybe @ros2/team has a different opinion.

@YoshuaNava
Copy link
Author

@clalancette thank you for your prompt reply.

At the moment I'm writing utility code to perform parameter fetching and validation, and I had to write two different functions for rclcpp and builtin_interfaces Time. This immediately confused my project partners, who would be downstream users of the code I'm writing.

Can you give some background on what would you see as blocking for this change?

@clalancette
Copy link
Contributor

Can you give some background on what would you see as blocking for this change?

Doing some simple, non-exhaustive looks through rolling right now, I see that there are 177 references to builtin_interfaces::msg::Time spread across 67 files. There are 3145 references to rclcpp::Time spread across 888 files.

If we make a change to either of them, then someone is going to have to go through every single one of those files and make sure that they work with the new change. And this only counts the packages that are released; there are thousands of packages that haven't been, which we'd be introducing breakage for.

While this is certainly possible to do, I really don't think it is worth the effort. As I said, in my opinion both are clear, even if they are unfortunately different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants