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

Fix rosfish #172

Merged
merged 37 commits into from Aug 15, 2018

Conversation

Projects
None yet
3 participants
@amiller27
Copy link
Contributor

commented Mar 13, 2018

The rosfish script here is missing a lot of functionality (and some things just plain didn't work, possibly because fish has changed a lot since this was written). I've gone and ported much of rosbash straight into fish and put it here. Some things are still not implemented or are not as fast as I would like them to be. However, the autocomplete for the main commands that I tend to use (and I assume most others tend to use) is working. I can add the rest of the functionality if it's necessary in order to get this merged.

@@ -354,5 +788,14 @@ complete -x -c rosrun -a '(_roscomplete_rosrun)'
complete -x -c roslaunch -a '(_roscomplete_launch)'
complete -x -c rosed -a '(_roscomplete_file)'
complete -c rosed -l help --description "prints command info"
complete -x -c roscp -a '(_roscomplete_file)'
#complete -x -c roscp -a '(_roscomplete_file)'et -x TEST works

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Mar 13, 2018

Member

This change looks weird. Please check why it is commented out and what the appended string is.

This comment has been minimized.

Copy link
@amiller27

amiller27 Mar 13, 2018

Author Contributor

Sorry, that's because I started working on this before #171. I commented it out (because that line isn't valid fish, as fixed in #171), the appended string is gibberish which was in there before #171 for some reason. I'll fix it.

This comment has been minimized.

Copy link
@amiller27

amiller27 Mar 13, 2018

Author Contributor

The reason I left it commented out is because the implementation of roscp here doesn't work (and doesn't look like it should work in fish to me), and I haven't fixed it. I'll remove the gibberish though. Has this script (before this pr) been tested by anyone with a recent version of fish?

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

Since you are already reformatting existing code maybe it makes sense to use a consistent indentation throughout the file?

@amiller27

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

I agree, it's all 4-space indents now.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

Thank you for the patch.

Some things are still not implemented or are not as fast as I would like them to be.
I can add the rest of the functionality if it's necessary in order to get this merged.

Would you like to get this merged as-is or continue adding more to this PR ( I don't mind either or).

@amiller27

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

I have a few more commands I'm working on right now, I'll ping you when those are done. My guess is that it'll be sometime in the next week, and we can merge it then.

@amiller27

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2018

@dirk-thomas This is ready to go. All the functions are implemented now, and everything that's there works. There are a couple TODO's still scattered around, but they're minor (in terms of functionality they add) and I'm not going to tackle them in this PR.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

This is ready to go

Since you kept committing more changes I was holding off from merging. If you plan do apply further changes the PR can stay open for longer. Just let me know when you think it should be merged.

@amiller27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2018

Yeah thanks. I thought it was done and then I discovered some minor bugs/improvements through normal use. I think I'll sit on this for a little while and see if I come up with anything else, I'll ping you again when I'm done with it.

@amiller27 amiller27 force-pushed the amiller27:fix-fish branch from 37c25a6 to 50a6847 May 7, 2018

amiller27 added some commits May 7, 2018

@nim65s

This comment has been minimized.

Copy link

commented Aug 14, 2018

Hello :)
Any news about this ?
Do you need help or some tests, or anything before merging ?

@amiller27

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

@dirk-thomas @nim65s This should be ready to merge. I haven't found any more bugs

@dirk-thomas dirk-thomas merged commit 973feca into ros:kinetic-devel Aug 15, 2018

3 checks passed

Kpr__ros__ubuntu_xenial_amd64 Build finished.
Details
Lpr__ros__ubuntu_xenial_amd64 Build finished.
Details
Mpr__ros__ubuntu_bionic_amd64 Build finished.
Details
@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

Thank you for contributing this patch.

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.