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 ros2 launch #147
Fix ros2 launch #147
Conversation
Please run code you change to confirm it works as expected. |
argcomplete it doesn't support Windows. Moved the `argcomplete` outside the try catch Signed-off-by: ahcorde <ahcorde@gmail.com>
07762e4
to
4ba8cf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (haven't tried it though).
I tried on Windows and Ubuntu Focal. is it enough to run CI up-to ros2lanch and run only these tests? |
I don't think any unit test uses |
I ckecked other PR related with argcomplete #126, it's only testing ros2launch. I also checked CI (testing just ros2launch) |
I think this fixes an issue on all platforms (not just Windows). I've run into it on Linux. The code before this patch assumes that argcomplete is installed, but it is optional according to our docs. Trying
|
try: | ||
from argcomplete.completers import FilesCompleter | ||
except ImportError: | ||
# argcomplete is not supported on Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: A better comment might indicate that argcomplete is optional
# argcomplete is not supported on Windows | |
# argcomplete is optional |
I can confirm this PR resolves my issue on Linux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest updating the PR title and/or the comments in the code to indicate this patch is making argcomplete optional.
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-foxy-fitzroy-call-for-testing-and-package-releases/13998/1 |
Signed-off-by: ahcorde <ahcorde@gmail.com>
thank you @jacobperron |
As @dirk-thomas indicates in this comment
Moved the
argcomplete
outside the try catch.