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

Extend ROS resource addressing design document #241

Open
wants to merge 3 commits into
base: gh-pages
Choose a base branch
from

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jul 4, 2019

This pull request generalizes the existing topic and service name addressing design document to cover the current goals in terms of addressing topics, services, actions and parameters in ROS 2.

@hidmic hidmic requested a review from wjwwood July 4, 2019 15:38
@hidmic
Copy link
Contributor Author

hidmic commented Jul 4, 2019

As a follow-up, the design document on static name remapping will need an update as well.

@hidmic hidmic changed the title Refactor and extend ROS resource addressing design document. Extend ROS resource addressing design document. Jul 4, 2019
@hidmic hidmic changed the title Extend ROS resource addressing design document. Extend ROS resource addressing design document Jul 4, 2019
articles/140_resource_names.md Show resolved Hide resolved
articles/140_resource_names.md Show resolved Hide resolved
articles/140_resource_names.md Outdated Show resolved Hide resolved
articles/140_resource_names.md Outdated Show resolved Hide resolved
articles/140_resource_names.md Outdated Show resolved Hide resolved
articles/140_resource_names.md Outdated Show resolved Hide resolved
articles/140_resource_names.md Outdated Show resolved Hide resolved
- must not start with a numeric character (`[0-9]`).
- must not end nor start with a period (`.`).
- must not contain any number of repeated periods (`.`).
- must not contain any number of repeated underscores (`_`).
Copy link
Member

Choose a reason for hiding this comment

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

So this is back :D. We should figure out what we're doing here and be consistent. Unless I missed it this wasn't a constraint mentioned above. I think we may have wanted to keep it to allow us to have a "safe" delimiter to use in the future, but it looks like the code doesn't check this, at least for topics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this constraint explicit everywhere else in 7680627. Do you think we shouldn't?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, not sure. I think we only needed it for the alternative we're no longer using. I don't think the code enforces it at the moment. Maybe it should be removed. @sloretz or anyone on the @ros2/team do you have an opinion here?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong preference. But it seems okay to remove the constraint if there's no reason to have it. If we really need it in the future, we could start using repeated . as a delimiter since it's already disallowed.

articles/140_resource_names.md Outdated Show resolved Hide resolved
articles/140_resource_names.md Outdated Show resolved Hide resolved
- Enable redirects to avoid breaking existing permalinks
- Improve overall writing and internal consistency
- Added additional citations

Signed-off-by: Michel Hidalgo <[email protected]>
articles/140_resource_names.md Outdated Show resolved Hide resolved
articles/140_resource_names.md Outdated Show resolved Hide resolved
articles/140_resource_names.md Outdated Show resolved Hide resolved
- must not start with a numeric character (`[0-9]`).
- must not end nor start with a period (`.`).
- must not contain any number of repeated periods (`.`).
- must not contain any number of repeated underscores (`_`).
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong preference. But it seems okay to remove the constraint if there's no reason to have it. If we really need it in the future, we could start using repeated . as a delimiter since it's already disallowed.

@mjcarroll
Copy link
Member

@hidmic What is the status on this? Can it be merged or does it need to move forward to the Foxy board?

@hidmic
Copy link
Contributor Author

hidmic commented Jan 6, 2020

It has yet to be implemented, so if anything it has to go to the Foxy board. It was brought up during our brainstorming for Foxy, but AFAIK it has a rather low priority.

@hidmic
Copy link
Contributor Author

hidmic commented Mar 19, 2020

ros2/ros2#881 to track progress on this topic.

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

Successfully merging this pull request may close these issues.

5 participants