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.

Sign up for GitHub

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

Jump to bottom

Add support for Fish shell #326

Draft
wants to merge 5 commits into
base: rolling
Choose a base branch
from
Draft

Add support for Fish shell #326

wants to merge 5 commits into from

Conversation

ruffsl
Copy link
Member

@ruffsl ruffsl commented Sep 5, 2019

This adds completion support for fish shell: https://fishshell.com
Given that argcomplete works for fish as well: kislyuk/argcomplete#260

@ruffsl ruffsl changed the title Fish Add support for Fish shell Sep 5, 2019
dirk-thomas
dirk-thomas reviewed Sep 5, 2019
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably wait until fish support is available in various other parts of the pipeline (e.g. ament_package, colcon).

ros2cli/resource/local_setup.fish
@@ -0,0 +1,31 @@
# reduced from ament_package/template/package_level/local_setup.fish.in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file doesn't exist atm?

ros2cli/completion/ros2-argcomplete.fish
# limitations under the License.

if type register-python-argcomplete3 > /dev/null 2>&1
eval "register-python-argcomplete3 --shell fish ros2 | source"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using eval and source looks weird - are both really necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The . command is deprecated as the source command is preferred: fish-shell/fish-shell#310
But I suppose the eval might not be needed. I was just mimicking the style in bash file.
Why was eval used there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is the recommended approach described in the argcomplete docs: https://argcomplete.readthedocs.io/en/latest/#synopsis

@ruffsl
Copy link
Member Author

ruffsl commented Sep 5, 2019 •
edited
Loading

other parts of the pipeline (e.g. ament_package, colcon).

What else would to be need added or ported?
Here somewhere:
https://github.com/ament/ament_package
template/*/*.bash* -> template/*/*.fish*
https://github.com/colcon/colcon-bash -> colcon-fish

@dirk-thomas
Copy link
Member

What else would to be need added or ported?

I don't know out of my head. I would suggest to work on each of them incrementally and then see if anything else is missing.

@mabelzhang mabelzhang added the enhancement New feature or request label Sep 26, 2019
@mjcarroll
Copy link
Member

@sloretz FYI 🐟

@sloretz sloretz self-requested a review November 19, 2019 17:51
@gbiggs
Copy link
Member

gbiggs commented Jun 24, 2020

@ruffsl What needs to be done to get this out of draft status?

@ruffsl
Copy link
Member Author

ruffsl commented Jun 24, 2020

@gbiggs , it's been a while, but I think dirks's comment on support available for the rest of the pipeline still stands. #326 (review)

I can't remember what may still be missing here, but ros2cli and setup sh scripts may have changed some sense I started this draft.

@bergercookie
Copy link

How could I test this PR?
Is it enough checking out this branch when compiling ROS 2 from source, or do I need to make additional changes (or manual steps), e.g., in colcon?

@audrow audrow self-assigned this Mar 12, 2021
@NextAlone
Copy link

Is there any progress?

@audrow
Copy link
Member

audrow commented Mar 14, 2022 •
edited
Loading

Is there any progress?

I'm not aware of anything moving on this. If the merge conflicts are sorted out and it seems to work, then it'd be worth another review.

How could I test this PR? Is it enough checking out this branch when compiling ROS 2 from source, or do I need to make additional changes (or manual steps), e.g., in colcon?

I think you're going to want to rebase this PR and then sort out any merge conflicts that remain. After that, you should be able to build from source and test it out.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:23
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
@wjwwood
RosTimer -> ROSTimer and PushRosNamespace -> PushROSNamespace, to fol…
73098cf 
…low PEP8 (ros2#326)

Signed-off-by: William Woodall <[email protected]>

Signed-off-by: William Woodall <[email protected]>
@alvgaona alvgaona mentioned this pull request Apr 25, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

@sloretz sloretz Awaiting requested review from sloretz

@audrow audrow Awaiting requested review from audrow audrow will be requested when the pull request is marked ready for review audrow is a code owner

@gbiggs gbiggs Awaiting requested review from gbiggs gbiggs will be requested when the pull request is marked ready for review gbiggs is a code owner

Assignees

@audrow audrow

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ruffsl @dirk-thomas @mjcarroll @gbiggs @bergercookie @NextAlone @audrow @mabelzhang