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

rosbash: Fix zsh rosservice completion. #92

Merged
merged 1 commit into from Oct 7, 2015

Conversation

Projects
None yet
3 participants
@de-vri-es
Copy link
Contributor

commented Oct 6, 2015

I had to change this for rosservice completion to work correctly. ${words[-1]} is the message value (possibly empty string) at this point and not the service name. I changed it to ${words[3]}. ${words[-2]} should also work, but since the subcommand is already hardcoded as ${words[2]} and there are no optional options I saw no harm in hardcoding index 3 as well.

Also I am very curious why the words array is being accessed sometimes as ${=${(s: :)words[x]}}. It seems like a very elaborate NOP to me. Why do that over ${words[x]}?

Alternatively ${=${(s: :)words}[x]} is not a NOP (though the second split might be) but seems plain wrong to me. We should not be doing word splitting at all, the shell has already done that while taking into account quotes and escape characters and what not. Flattening the array into a string and then splitting on spaces is simply wrong, not to mention more complex and hard to read.

/edit:

The complection that was not working was:
rosservice call /some/service [tab]

It should give the layout of the request body so you can fill in details. However, it just wrote errors to stderr. After the PR it should fill in the request body.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Oct 6, 2015

Near as I can tell this style of dereferencing the "words" was introduced here: 333f347

After that everyone seems to just have mimicked the style. I'm a little wearing changing it, since I'm not sure why it was done in the first place.

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2015

I see. Thanks for the pointer.

@kwc: Could you elaborate why you decided to split $words like that? Is there something I overlooked?

@@ -740,7 +739,7 @@ compctl -K "_roscomplete_rosbag" + -g "*.bag *(/)" "rosbag"
compctl -K "_roscomplete_rosnode" "rosnode"
compctl -K "_roscomplete_rosparam" "rosparam"
compctl -x 'p[0,2]' -K "_roscomplete_rostopic" - 'n[1,/] p[3]' -K "_roscomplete_rostopic" - 'p[3]' -S ' ' -K "_roscomplete_rostopic" - 'p[4]' -Q -K "_roscomplete_rostopic" -- "rostopic"
compctl -K "_roscomplete_rosservice" "rosservice"
compctl -Q -K "_roscomplete_rosservice" "rosservice"

This comment has been minimized.

Copy link
@wjwwood

wjwwood Oct 6, 2015

Member

Why did you add -Q?

This comment has been minimized.

Copy link
@de-vri-es

de-vri-es Oct 6, 2015

Author Contributor

-Q prevents zsh from escaping special characters like quotes and spaces in the completion candidates. Since rosmsg-proto already gives output suitable for parsing by the shell, escaping it just makes it illegible

@wjwwood

This comment has been minimized.

Copy link
Member

commented Oct 6, 2015

@de-vri-es, @kwc may not have time to answer, but we can try to reason about why that be necessary. I'm not opposed to changing it, but it would be great if we could come up with a reason as to why someone might do that. It also looks like @kwc may have just merged someone else's patch, so it might not be something he did originally.

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2015

(should have commented in the line notes)

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2015

@wjwwood: I see. I can't really think of a reason though. What will happen is that arguments containing spaces on the command line (escaped with a backslash or quotes) will be turned into separate array elements. In the original $words array, the argument will be just one array element, even if it contains spaces.

However, the behaviour of the original $word array without splitting seems correct to me. After all, that array truly represents the separate command line arguments.

I can test locally with just ${word[i]} and see if anything breaks. I doubt I will trigger every code path during my normal usage though.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Oct 6, 2015

@de-vri-es I think it would be ok to take the pr as proposed after we test it locally, but I'll let @dirk-thomas voice an opinion before doing that.

In the mean time, could you give an example of what completion was not working before and that works after this patch so that I can try it on my machine?

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2015

@wjwwood: Ah yes, should have done that in the PR.

The complection that was not working was:
rosservice call /some/service [tab]

It should give the layout of the request body so you can fill in details. However, it just wrote errors to stderr. After the PR it should fill in the request body.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Oct 6, 2015

I just tested this against indigo on my mac and I can confirm the before and after behavior.

@dirk-thomas dirk-thomas changed the title robash: Fix zsh rosservice completion. rosbash: Fix zsh rosservice completion. Oct 7, 2015

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

Same for me with zsh on Ubuntu.

Thank you for the patch.

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

Merge pull request #92 from de-vri-es/jade-devel
rosbash: Fix zsh rosservice completion.

@dirk-thomas dirk-thomas merged commit 3a87819 into ros:jade-devel Oct 7, 2015

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2015

@wjwwood @dirk-thomas: Thanks for the quick tests and merge :)

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.