Skip to content

override argument parser to fallback to args rather than options#63

Merged
dirk-thomas merged 4 commits intomasterfrom
patch_argument_parser
Jun 1, 2018
Merged

override argument parser to fallback to args rather than options#63
dirk-thomas merged 4 commits intomasterfrom
patch_argument_parser

Conversation

@dirk-thomas
Copy link
Copy Markdown
Member

Fixes #36.

This is an "interesting" approach to monkey patch the change proposed upstream.

I will put this in review for people to try and comment on the patch. Once it is approved the help texts can be updated to mention the space prefix only for cases the the arguments are actually valid options of colcon.

@dirk-thomas dirk-thomas added enhancement New feature or request review Waiting for review (Kanban column) labels May 31, 2018
@dirk-thomas dirk-thomas added this to the 0.2.2 milestone May 31, 2018
@dirk-thomas dirk-thomas self-assigned this May 31, 2018
@dirk-thomas
Copy link
Copy Markdown
Member Author

While I picked @mikaelarguedas as the reviewer I hope others will give it a try to and review the patch to move forward on this quickly.

@dirk-thomas dirk-thomas force-pushed the patch_argument_parser branch from 7c0e15e to a555aa9 Compare May 31, 2018 22:06
@codecov-io
Copy link
Copy Markdown

codecov-io commented May 31, 2018

Codecov Report

Merging #63 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   77.23%   77.28%   +0.05%     
==========================================
  Files          49       49              
  Lines        2609     2615       +6     
  Branches      420      421       +1     
==========================================
+ Hits         2015     2021       +6     
  Misses        571      571              
  Partials       23       23
Impacted Files Coverage Δ
colcon_core/task/python/test/pytest.py 0% <ø> (ø) ⬆️
colcon_core/command.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09c9362...e54d430. Read the comment docs.

@mikaelarguedas
Copy link
Copy Markdown
Contributor

colcon build --packages-select console_bridge --cmake-args -DBUILD_TESTING=ON -DFOOBAR=OFF
CMake Warning:
  Manually-specified variables were not used by the project:

    BUILD_TESTING
    FOOBAR


---

🎉 works great !

Though the arguments following the greedy arguments don't seem to be detected as I expected:

For example, here I was expecting a symlink install:

colcon build --packages-select console_bridge --cmake-args -DBUILD_TESTING=ON -DFOOBAR=OFF  --symlink-install
-- Installing: /home/mikael/work/ros2/bouncy_ws/install/console_bridge/lib/console_bridge/cmake/console_bridge-targets-release.cmake
-- Installing: /home/mikael/work/ros2/bouncy_ws/install/console_bridge/lib/console_bridge/cmake/console_bridge-config.cmake
-- Installing: /home/mikael/work/ros2/bouncy_ws/install/console_bridge/lib/console_bridge/cmake/console_bridge-config-version.cmake
-- Installing: /home/mikael/work/ros2/bouncy_ws/install/console_bridge/lib/pkgconfig/console_bridge.pc

Is there some kind of delimimter that should be used ?

@dirk-thomas
Copy link
Copy Markdown
Member Author

For example, here I was expecting a symlink install:

It seemed to work for options following the greedy one in my local testing. Can you double check the logs what args have been parsed.

Is there some kind of delimimter that should be used ?

No delimiter.

@dirk-thomas
Copy link
Copy Markdown
Member Author

dirk-thomas commented May 31, 2018

Also console_bridge is a plain CMake package, no? I doubt that can do a symlink install in the first place...

@mikaelarguedas
Copy link
Copy Markdown
Contributor

Oh yeah that could be it, I took the first non-python package from my topological order ^^ poor choice...

Works fine with class_loader 👍 sorry for the noise

-- Symlinking: /home/mikael/work/ros2/bouncy_ws/install/class_loader/include/class_loader/multi_library_class_loader.hpp
-- Symlinking: /home/mikael/work/ros2/bouncy_ws/install/class_loader/include/class_loader/register_macro.hpp
-- Symlinking: /home/mikael/work/ros2/bouncy_ws/install/class_loader/include/class_loader/visibility_control.hpp

@mikaelarguedas
Copy link
Copy Markdown
Contributor

I didn't get a chance to test many configurations but, from the few scenarios I tested, this PR works perfectly 👍
(tested only cmake-args and ctest-args so far)

Copy link
Copy Markdown
Contributor

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm, feel free to merge or wait for more testing/feedback from @ros2/team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request review Waiting for review (Kanban column)

Development

Successfully merging this pull request may close these issues.

consider custom parsing logic to avoid leading space for nested arguments

3 participants