-
Notifications
You must be signed in to change notification settings - Fork 412
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
Sub Node alternative #581
Merged
Merged
Sub Node alternative #581
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
1190e30
Sub Node alternative
fmrico a34520b
Sub Node alternative
fmrico 65330d3
Test // characters in namespaces
fmrico c2edea1
Sub Node alternative
fmrico 9522e4b
Test // characters in namespaces
fmrico 98e5406
Fixing style and warning in the order of initalizing members
fmrico 1aa37ad
Fixing cases with / in different positions, and adding new tests
fmrico 96acea0
Removing commented methods
fmrico 951c728
Changing extended_namespace to sub_namespace
fmrico 7241628
Merged with origin
fmrico 00ee3b1
Merge branch 'master' of https://github.com/ros2/rclcpp into get_node
fmrico dfcc5bf
Fixed a bug when merging
fmrico 6c2e5e2
Fixed a bug when merging
fmrico 7f8371d
Sub Node alternative
fmrico 98f2b07
Sub Node alternative
fmrico b8e7997
Test // characters in namespaces
fmrico 3d742b1
Fixing style and warning in the order of initalizing members
fmrico f72f74a
Fixing cases with / in different positions, and adding new tests
fmrico 62c28de
Removing commented methods
fmrico 15f3480
Changing extended_namespace to sub_namespace
fmrico f878c3f
Fixed a bug when merging
fmrico fe2c6b6
Merge with origin
fmrico 090d2ae
Merge with origin to update branch
fmrico dbebb1f
improvements to API and documentation
wjwwood 7ea7269
style and fixing tests
wjwwood 014a16f
fixup subnode specific tests
wjwwood 6ef0389
remove vestigial function
wjwwood 8b17568
improve documentation
wjwwood 69155fd
add test to check interaction between ~ and sub-nodes
wjwwood 25d346b
typo
wjwwood File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could you maybe comment a little more on the split between
get_namespace
,get_sub_namespace
, andget_effective_namespace
? I understand that they are different (and I think I even understand their differences), but I'm concerned that this set of APIs is going to be confusing for users to use. When should a user call each of them?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 felt like I had described their difference in the doc blocks, but maybe it needs clarification.
Briefly,
get_namespace()
returns the node's namespace (which is unaffected by any sub-namespaces),get_sub_namespace()
returns the current sub-namespace, andget_effective_namespace()
is the namespace that will effectively be used by entities (like pub/sub/client/server) when being created.I don't expect the user to ever need to call these functions unless they're doing something complicated or their using it to construct some very specific logging functions.
get_effective_namespace()
is purely a convenience, since it's actually not used when creating entities, so I guess we could eliminate it. But the other two, return information that is originally given by the user and that the node needs to store, so I don't see a reason to not allow the user to get the information back out of the node (keep the members private but remove the accessors).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.
Yeah, as mentioned, I basically understood that from reading the code. My biggest worry is that exposing these APIs to the user is going to result in a lot of answers.ros.org questions of the form: when do I call which of these APIs? On the other hand, you seem to be saying that these probably won't need to be called except in specialized circumstances, so maybe that concern is unwarranted. It's not a blocker for me, so I won't press the point further.
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 understand the concern, I still would error on the side of exposing information. At some level I think it’s good if this leads to questions about which to call, as that forces people to learn about the options, which I’d prefer to them calling
get_namespace()
but receiving something different from what they assumed.Thanks for iterating, if you or anyone comes up with a better way to organize it that might be less confusing but still provides access to available data, I’m all ears.