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

Fix #96 Add EBNF grammar for ROS2 names in self-testing python script #97

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from
Open

Conversation

tkruse
Copy link

@tkruse tkruse commented Oct 30, 2016

Prototype for #96. Since last time I extended a bit. Because of the fully-qualified vs. non-fully-qualified, the grammar has grown a bit.

The grammar still does not prevent double underscores, the python script around it can handle that much easier I think.

The script itself does not yet handle maximum name length constraints.

@tkruse tkruse changed the title Fix #90 Add EBNF grammar for ROS2 names in self-testing python script Fix #96 Add EBNF grammar for ROS2 names in self-testing python script Oct 30, 2016
@tkruse
Copy link
Author

tkruse commented Oct 30, 2016

Some things to clarify (consider clarifying in the article):

  • is '{foo}' only allowed? -> current script assumes yes
  • is /foo/123/bar allowed? -> current script assumes yes
  • "balanced" curly braces may imply nesting? -> current script assumes no
  • ~ only in starting position? -> current script assumes yes, only in starting position
  • url scheme only 'rostopic' and 'rosservice' allowed? -> current script allows any alphabetic name

@wjwwood
Copy link
Member

wjwwood commented Oct 31, 2016

Thanks for putting the current state into a pr, that's easier to track.

is {foo} only allowed? -> current script assumes yes

I'd say yes, so long as the contents of foo are valid on their own, i.e. not empty.

is /foo/123/bar allowed? -> current script assumes yes

I would say no, the section for name tokes says:

https://github.com/ros2/design/blob/gh-pages/articles/115_topic_and_service_name_mapping.md#name-tokens

Topic and service name tokens:

...

  • must not start with numeric characters ([0-9])

Since 123 in your example is a token, I think it must start with an alphabetical character.

"balanced" curly braces may imply nesting? -> current script assumes no

I suppose that it could, but the document does not appear to be clear on that point right now. I vaguely remember discussing it, but I don't see it in the issue history at the moment, so I'll open an issue to address that concern: #98

~ only in starting position? -> current script assumes yes, only in starting position

Yes, that is described in the document I as:

https://github.com/ros2/design/blob/gh-pages/articles/115_topic_and_service_name_mapping.md#ros-2-topic-and-service-name-constraints

For convenience here is a summary of all rules for topic and service names in ROS 2:
...

  • may contain alphanumeric characters ([0-9|a-z|A-Z]), underscores (_), or forward slashes (/)
  • ...
  • may start with a tilde (~), the private namespace substitution character

It is also mentioned later in the document in more explicit terms.

url scheme only 'rostopic' and 'rosservice' allowed? -> current script allows any alphabetic name

I think that is find for the script, for now. I imagine we'll want to limit what goes there, but I also don't think rostopic and rosservice will be exhaustive in the end. We can leave it open ended for now (like with the substitution names).

@wjwwood
Copy link
Member

wjwwood commented Oct 31, 2016

I just wanted to clarify my response to:

is /foo/123/bar allowed? -> current script assumes yes

The document says, more specifically:

https://github.com/ros2/design/blob/gh-pages/articles/115_topic_and_service_name_mapping.md#name-tokens

Topic and service name tokens:

  • ...
  • may use alphanumeric characters ([0-9|a-z|A-Z]), single underscores (_), and/or balanced curly braces ({})
  • must not start with numeric characters ([0-9])

So, it cannot start with a numeric (so 123 is not a valid token name), but it can start with more than just an alpha, i.e. _ underscore or { curly brace.

Thanks @dhood for pointing this out.

@tkruse
Copy link
Author

tkruse commented Nov 1, 2016

Ah, somehow I missed the "token" bit. That makes the EBNF simpler to write. Updated the PR, added more positive and negative examples to tests.

@wjwwood wjwwood self-assigned this Apr 10, 2017
@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Actively being worked on (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants