-
Notifications
You must be signed in to change notification settings - Fork 301
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
[backport dashing] fix showing duplicate keys in --print-pairs #230
Conversation
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@@ -58,10 +58,10 @@ get_2to1_mapping( | |||
const std::string & ros2_type_name, | |||
std::string & ros1_type_name); | |||
|
|||
std::map<std::string, std::string> | |||
std::multimap<std::string, std::string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API change is necessary to get the necessary information to fix --print-pairs
. Either we are ok with that API breakage (and notify users in the sync email about it) or we should close this backport and leave the --print-pairs
in the broken state in Dashing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for closing to avoid API changes, or add a new function (different name) only in dashing and update our code to use that one instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes a public interface and so is not ABI/API compatible with Dashing.
In the interest of not breaking ABI without a strong justification, I think we need to leave the bug here (and log it in the Known Issues) but if consensus among @ros2/team is that breaking ABI to fix the bug is worth it I will rescind my objection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like a critical bug, so I'd say it's not worth breaking API/ABI.
Ok, with 3 x 👎 I will close this backport. Thanks for the feedback. |
Backport of #225.