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 parsing of cmd line arguments in XML and yaml file #379

Merged
merged 5 commits into from
Feb 17, 2020

Conversation

ivanpauno
Copy link
Member

Fixes #337.

The rules are a bit tricky, but I added a bunch of test cases. I will add any other suggested test case if needed.

I first though about solving the problem by addressing #263.
But I later realized that:

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Feb 14, 2020
@ivanpauno ivanpauno self-assigned this Feb 14, 2020
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments and suggestions.

launch/launch/actions/execute_process.py Outdated Show resolved Hide resolved
launch/launch/actions/execute_process.py Show resolved Hide resolved
launch/test/launch/frontend/test_substitutions.py Outdated Show resolved Hide resolved
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

The code is a bit complex, but I don't have suggestions for making it better.
LGTM with green CI.

@ivanpauno
Copy link
Member Author

  • Linux Build Status

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

Fixed linter failures in 6e74c91, the other failures are also happening in nightlies.

The code is a bit complex, but I don't have suggestions for making it better.

It would be possible to tweak the grammar and add a different Transformer to get directly a list of arguments, instead of manually converting one thing to the other.

@ivanpauno ivanpauno merged commit f243fc6 into master Feb 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/fix-substitutions-in-cmd-argument branch February 17, 2020 17:25
ivanpauno added a commit that referenced this pull request Apr 16, 2020
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
ivanpauno added a commit that referenced this pull request Apr 16, 2020
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[launch_frontend] FindPackage substitution not concatenated with surrounding text
3 participants