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

Allow 'get_const_expr_value' to parse either literals or scoped_names… #430

Merged
merged 7 commits into from Jan 16, 2020

Conversation

kylemarcey
Copy link
Contributor

@kylemarcey kylemarcey commented Jan 4, 2020

  • Changed the function get_const_expr_value in parser.py to work when the const expr is a scoped_name as well as when it is a literal

Fixes #429.

@dirk-thomas
Copy link
Member

Please also update the unit tests to cover the newly supported case.

kylemarcey added a commit to kylemarcey/rosidl that referenced this pull request Jan 10, 2020
Signed-off-by: kyle.marcey <kyle.marcey@apex.ai>
@ivanpauno ivanpauno added enhancement New feature or request in review Waiting for review (Kanban column) in progress Actively being worked on (Kanban column) bug Something isn't working and removed in review Waiting for review (Kanban column) enhancement New feature or request labels Jan 16, 2020
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
… the comment and output the whole tree in case of unsupported case

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member

dirk-thomas commented Jan 16, 2020

Thanks for the patch. In the future please comment on the PR after pushing commits - otherwise nobody is being notified that the PR needs another look.

I added a few commits:

  • fix a linter warning (please make sure to run the tests locally)
  • check len of children before accessing them
  • since the added logic already identifies the literal remove the previous logic to find that node
  • removed one comment since the passed expression could contain other expressions which isn't supported yet (and update the assertion message to print the whole subtree for better debugging)

I hope these changes make sense to you.

CI build on Linux only: Build Status

@kylemarcey
Copy link
Contributor Author

Yes, those changes make sense to me, thanks!

@dirk-thomas dirk-thomas removed the in progress Actively being worked on (Kanban column) label Jan 16, 2020
@dirk-thomas dirk-thomas merged commit 29ad520 into ros2:master Jan 16, 2020
@dirk-thomas dirk-thomas added this to Needs Backport in Eloquent Patch Release 1 Jan 16, 2020
@dirk-thomas dirk-thomas added this to Needs Backport in Dashing Patch Release 6 Jan 16, 2020
@dirk-thomas
Copy link
Member

Fast forwarded the change to the eloquent branch.

@dirk-thomas dirk-thomas moved this from Needs Backport to Needs Release in Eloquent Patch Release 1 Jan 16, 2020
dirk-thomas added a commit that referenced this pull request Jan 16, 2020
#430)

* Allow 'get_const_expr_value' to parse either literals or scoped_names (#429)

Signed-off-by: kyle.marcey <kyle.marcey@apex.ai>

* Add test case for annotation using scoped_name (#430)

Signed-off-by: kyle.marcey <kyle.marcey@apex.ai>

* fix linter warning

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* check len of children before accessing them

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* nitpick: swap comparison sides

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* avoid finding single literal again

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* since const_expr could contain different expressions (e.g. or) remove the comment and output the whole tree in case of unsupported case

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas removed this from Needs Backport in Dashing Patch Release 6 Jan 16, 2020
dirk-thomas added a commit that referenced this pull request Jan 16, 2020
#430) (#431)

* Allow 'get_const_expr_value' to parse either literals or scoped_names (#429)

Signed-off-by: kyle.marcey <kyle.marcey@apex.ai>

* Add test case for annotation using scoped_name (#430)

Signed-off-by: kyle.marcey <kyle.marcey@apex.ai>

* fix linter warning

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* check len of children before accessing them

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* nitpick: swap comparison sides

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* avoid finding single literal again

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* since const_expr could contain different expressions (e.g. or) remove the comment and output the whole tree in case of unsupported case

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

Co-authored-by: kylemarcey <marcey.kyle@gmail.com>
@mjcarroll mjcarroll moved this from Needs Release to Released in Eloquent Patch Release 1 Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[rosidl_parser] Some IDL attributes cause rosidl_parser to fail
3 participants