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

add object representation and conversion from ast #326

Merged
merged 2 commits into from
Nov 21, 2018
Merged

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Nov 15, 2018

This is the fifth PR integrating #298 step-by-step.

Builds on top of #325. Needs ros2/ci#220.

After lark parses any input file the AST is converted to an object representation (which will later be passed into the message generator templates).

Linux CI: Build Status

@dirk-thomas dirk-thomas added enhancement New feature or request in review Waiting for review (Kanban column) labels Nov 15, 2018
@dirk-thomas dirk-thomas self-assigned this Nov 15, 2018
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Partial review. I have some questions about the definitions. With those answers I'll be better able to review the parser.

rosidl_parser/test/test_parser.py Outdated Show resolved Hide resolved
rosidl_parser/rosidl_parser/definition.py Show resolved Hide resolved
rosidl_parser/rosidl_parser/definition.py Show resolved Hide resolved
rosidl_parser/rosidl_parser/definition.py Show resolved Hide resolved
rosidl_parser/rosidl_parser/definition.py Show resolved Hide resolved
rosidl_parser/rosidl_parser/definition.py Show resolved Hide resolved
rosidl_parser/rosidl_parser/definition.py Outdated Show resolved Hide resolved
@dirk-thomas
Copy link
Member Author

Just another CI after the changes:

@dirk-thomas
Copy link
Member Author

Sill good with the common.lark file removed:

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

LGTM for now with a minor fix. There are limitations that should be ticketed like includes in modules and different constants in different messages. Tests with IDL files that are acceptable to the grammar but illogical, like a boolean constant being assigned a string value, would be good too, but that can be ticketed rather than blocking the remaining PRs.

rosidl_parser/test/test_parser.py Show resolved Hide resolved
@dirk-thomas
Copy link
Member Author

Thanks. I created #329 for the followup improvements.

Squashing the commits before the merge...

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

green check mark

@dirk-thomas
Copy link
Member Author

green check mark

That is very suspicious because the PR job has no way of pulling the lark-parser until we have a Debian package for it... 🤔

@dirk-thomas dirk-thomas force-pushed the idl-stage-5 branch 2 times, most recently from 9b6076f to 49b1eaa Compare November 19, 2018 17:19
@dirk-thomas
Copy link
Member Author

Full build: Build Status

@dirk-thomas
Copy link
Member Author

@ros-pull-request-builder retest this please

@dirk-thomas
Copy link
Member Author

Waiting for the python3-lark-parser Debian package...

@dirk-thomas
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas dirk-thomas merged commit f56405d into master Nov 21, 2018
@dirk-thomas dirk-thomas deleted the idl-stage-5 branch November 21, 2018 01:36
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants