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] Add roscat to display file contents as stdout #99

Merged
merged 1 commit into from Mar 9, 2016

Conversation

Projects
None yet
4 participants
@wkentaro
Copy link
Contributor

commented Dec 21, 2015

In most of my cases, I don't want to edit file but just view.
That's why I'm adding this function, the candidates of names:

  • rosless
  • rosview

@wkentaro wkentaro changed the title [rosbash] Add rosless to view files with pager [rosbash] Add rosview to view files with pager Dec 21, 2015

@wkentaro wkentaro force-pushed the wkentaro:add-rosless branch 2 times, most recently from e35ffbf to fc5d8dd Dec 21, 2015

wkentaro added a commit to wkentaro/jsk_common that referenced this pull request Dec 21, 2015

[jsk_tools] Add rosview
Upstream PR: ros/ros#99

Modified:
  - jsk_tools/env-hooks/99.jsk_tools.bash
  - jsk_tools/env-hooks/99.jsk_tools.zsh

@wkentaro wkentaro referenced this pull request Dec 21, 2015

Merged

[jsk_tools] Add rosview #1320

@tfoote

This comment has been minimized.

Copy link
Member

commented Dec 21, 2015

Starting with the simpler problem of just displaying the file might it be cleaner. Just implement roscat and then if you want to view it paginated you could do roscat foo bar | less or rosless could be implemented extending that capability.

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2015

That would be better 👍

@wkentaro wkentaro force-pushed the wkentaro:add-rosless branch 2 times, most recently from 76035f9 to f5fcc9a Dec 21, 2015

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2015

I updated the commit.

@wkentaro wkentaro force-pushed the wkentaro:add-rosless branch from f5fcc9a to e7971cd Dec 21, 2015

@tfoote

This comment has been minimized.

Copy link
Member

commented Mar 1, 2016

That looks a lot cleaner. @dirk-thomas it looks good to me but I haven't had a chance to test it.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 1, 2016

I tried rosview rospy_tutorials listener in bash which didn't output anything. I also tried zsh with the command rosview rospy_tutorials listener which printed That file does not exist in that package.. So I don't think this is ready to be merged.

I also think rosview ... which simply does roscat ... | less is not necessary. We don't have to reinvent every command line tool to save the user to type a handful of letters on the console.

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2016

I fixed the code for rosbash.
Actually I cannot reproduce the problem on zsh.

Also should I remove roscat?

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

I would suggest to remove rosview. But lets see that others think, @ros/ros_team ?

@tfoote

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

I think roscat is valuable, since there's no quick way to view files. But I agree that rosview seems redundant with roscat ... | less and is probably worth avoiding the extra complexity for future maintenance.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

I'd give +1 to both commands. While I understand that rosview can easily be represented with an alias or simply typing roscat <args> | less and that there is some maintenance cost, I still think taking the contribution is fine.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

I have tried roscat again and it works well when passing two argument in bash ans zsh (I haven't tested fish). But when calling roscat without arguments it prints Couldn't find package [] and also has a return code of zero. I think in that case it should print the usage as well as return 1 (since --help was not explicitly requested). Looking at the patch it seems to aim for that behavior but the condition is wrong.

Regarding rosless: please remove it from the PR since only one out of three reviews seems to be in favor.

Thank you for iterating on this!

@wkentaro wkentaro force-pushed the wkentaro:add-rosless branch 2 times, most recently from 85a240c to 33c0184 Mar 2, 2016

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2016

Thanks, for the comments!
I have updated the commit. (removed rosview, added condition to show help when there're few arguments)

@@ -279,6 +279,26 @@ function roscp
cp $arg $argv[3]
end

function roscat
set -l arg
if test (count $argv) = 0

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Mar 2, 2016

Member

This condition hasn't been updated. Have you tried this case as it is? Does this work with fish?

This comment has been minimized.

Copy link
@wkentaro

wkentaro Mar 3, 2016

Author Contributor

Oops, I overlooked fish, will soon fix it.

2016年3月3日木曜日、Dirk Thomasnotifications@github.comさんは書きました:

In tools/rosbash/rosfish
#99 (comment):

@@ -279,6 +279,26 @@ function roscp
cp $arg $argv[3]
end

+function roscat

  • set -l arg
  • if test (count $argv) = 0

This condition hasn't been updated. Have you tried this case as it is?
Does this work with fish?


Reply to this email directly or view it on GitHub
https://github.com/ros/ros/pull/99/files#r54814099.

和田 健太郎 / Kentaro Wada
http://wkentaro.com

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2016

I updated the commit, but cannot try to use on fish because I don't know how to use ros with fish. (there's no setup.fish, is it?)

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 3, 2016

You can just source the file you modified directly in a fish shell and should be able to use the commands defined in there.

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2016

There is the below error even if on jade-devel branch.

$ rosed rospy_tutorials listener
test: unexpected argument at index 2: '/opt/ros/indigo'
test: Expected a combining operator like '-a' at index 2
That file does not exist in that package.

@wkentaro wkentaro changed the title [rosbash] Add rosview to view files with pager [rosbash] Add rosview to show file contents Mar 3, 2016

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 3, 2016

Can you please look into a fix for this?

This should be backported to Indigo once it is merged.

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2016

hmm, I can't find how to fix it.
I'm not familiar with fish shell..

@wkentaro wkentaro force-pushed the wkentaro:add-rosless branch 2 times, most recently from e1b7ca5 to c19eb7a Mar 4, 2016

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2016

I removed the code for fish.
I can't believe the rosfish is working:

wkentaro@anaconda ~/r/i/s/r/r/t/rosbash> exec fish --login
wkentaro@anaconda ~/r/i/s/r/r/t/rosbash> . rosfish
complete: Too many arguments
/home/wkentaro/ros/indigo/src/ros/ros/tools/rosbash/rosfish (line 357): complete -x -c roscp -a '(_roscomplete_file)'et -x TEST works
                                                                        ^
in . (source) call of file “/home/wkentaro/ros/indigo/src/ros/ros/tools/rosbash/rosfish”,
    called on standard input,


       complete - edit command specific tab-completions

   Synopsis
       complete (-c|--command|-p|--path) COMMAND [(-s|--short-option)
       SHORT_OPTION] [(-l|--long-option|-o|--old-option) LONG_OPTION
       [(-a||--arguments) OPTION_ARGUMENTS] [(-d|--description) DESCRIPTION]

complete: Type “help complete” for related documentation
@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 9, 2016

The return code still needs some work. When running e.g. roscat roscpp_tutorials and the usage is printed it should not have a return code of 0. Same for calling it with a not existing file as well calling roscat without arguments.

But calling roscat --help should return 0.

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

Thanks, I fixed it.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 9, 2016

roscat roscpp_tutorials foo still returns 0.

@wkentaro wkentaro force-pushed the wkentaro:add-rosless branch from d3f9c94 to a52564d Mar 9, 2016

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

I fixed it. Please review again.

On Thu, Mar 10, 2016 at 2:30 AM, Dirk Thomas notifications@github.com
wrote:

roscat roscpp_tutorials foo still returns 0.


Reply to this email directly or view it on GitHub
#99 (comment).

和田 健太郎 / Kentaro Wada
http://wkentaro.com

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 9, 2016

That looks good now. Can you please squash the commits before the merge. Thank you!

@wkentaro wkentaro force-pushed the wkentaro:add-rosless branch from a52564d to 3a0d4d1 Mar 9, 2016

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

Squashed.

dirk-thomas added a commit that referenced this pull request Mar 9, 2016

Merge pull request #99 from wkentaro/add-rosless
[rosbash] Add rosview to show file contents

@dirk-thomas dirk-thomas merged commit 633ff6e into ros:jade-devel Mar 9, 2016

1 check passed

Jpr__ros__ubuntu_trusty_amd64 Build finished. 95 tests run, 0 skipped, 0 failed.
Details
@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 9, 2016

Cherry-picked to indigo-devel: 696efa7

@wkentaro wkentaro deleted the wkentaro:add-rosless branch Mar 9, 2016

@wkentaro

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

Thanks.

@wkentaro wkentaro changed the title [rosbash] Add rosview to show file contents [rosbash] Add roscat to display file contents as stdout Mar 9, 2016

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.