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

add rosconsole tab completion for bash #86

Merged

Conversation

Projects
None yet
3 participants
@cmansley
Copy link
Contributor

commented Jun 23, 2015

No description provided.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Jun 23, 2015

Looks reasonable to me, but I didn't try it out yet.

@cmansley

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2015

In trying it, I found a bug in rosconsole. See ros/ros_comm#631 But, the auto-complete works as expected.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

I just tried the patch and the completion itself looks good.

I ran into this but I don't recall what exactly I did:

Traceback (most recent call last):
  File "/home/dthomas/ros/indigo/ros_core/devel/bin/rosconsole", line 6, in <module>
    exec(fh.read())
  File "<string>", line 3, in <module>
  File "/home/dthomas/ros/indigo/ros_core/src/ros_comm/clients/rospy/src/rospy/rosconsole.py", line 165, in main
    _rosconsole_cmd_get(argv)
  File "/home/dthomas/ros/indigo/ros_core/src/ros_comm/clients/rospy/src/rospy/rosconsole.py", line 130, in _rosconsole_cmd_get
    if args[1] not in logger_level._current_levels:
AttributeError: 'LoggerLevelServiceCaller' object has no attribute '_current_levels'

Looking at the code it seems like LoggerLevelServiceCaller does not define any member variable in the constructor: https://github.com/ros/ros_comm/blob/134adfabc439cc006bed964697750802fcc36411/clients/rospy/src/rospy/logger_level_service_caller.py#L55

But the line at raises does use a member directly after instantiating the class.

Can you look into this and propose a fix? Eventually adding the following to the constructor is enough but I am not sure:

self._current_loggers = []
self._current_levels = {}
@cmansley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

Please read the previous comments. I already tagged this issue and submitted a pull request. ros/ros_comm#631

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

Sorry, I missed the referenced PR. (Still it looks fragile to not create member variables in the constructor if they are used outside.)

Can you please squash the two commits please. I will then merge it into jade and cherry-pick it to indigo.

@cmansley cmansley force-pushed the cmansley:rosconsole-bash-tab-completion branch from db40ad3 to 5f5e461 Aug 6, 2015

@cmansley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

I squashed down the commits. It should be ready for merge. Thanks!

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

Thanks. Cherry picked to indigo branch: c3825b4

dirk-thomas added a commit that referenced this pull request Aug 7, 2015

@dirk-thomas dirk-thomas merged commit bfd51ea into ros:jade-devel Aug 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.