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

Enable arguments with spaces in alias definition #476

Merged
merged 4 commits into from
Jul 19, 2017

Conversation

Xfel
Copy link
Contributor

@Xfel Xfel commented Jul 18, 2017

This is the solution for #464. It supports both using quotes as understood by shlex in the string and using a yaml list to specify the single arguments.

The only downside to this is that it uses the function pipes.quote which is apparently deprecated.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

The logic behind the changes lgtm. I also think you got all the places that needed to be touched (it's hard to be certain though).

However, one of the tests are still failing:

======================================================================
FAIL: tests.unit.test_config.test_get_verb_aliases
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.7.13/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/catkin/catkin_tools/tests/utils.py", line 137, in decorated
    return f(*args, **kwds)
  File "/home/travis/build/catkin/catkin_tools/tests/unit/test_config.py", line 80, in test_get_verb_aliases
    assert aliases['b'] == 'build'
AssertionError
----------------------------------------------------------------------

So I'll approve this when that's fixed.

Thanks!

@Xfel
Copy link
Contributor Author

Xfel commented Jul 19, 2017

Please excuse the two commits, I couldn't get the tests to run on my machine, so I had to push and let travis do the work. Those issues should be fixed now. I did however find another issue that is not covered by the tests, comes from a moved function between python2.7 and python3.4. What is the recommended way to deal with this issue?

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm now, thanks!

@wjwwood
Copy link
Member

wjwwood commented Jul 19, 2017

Please excuse the two commits, I couldn't get the tests to run on my machine, so I had to push and let travis do the work.

No worries, I'll squash with GitHub when merging.

@wjwwood wjwwood merged commit da79d37 into catkin:master Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants