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 tutorial for fish in execute_commands_on_host.md #257

Merged
merged 2 commits into from May 5, 2022
Merged

Add tutorial for fish in execute_commands_on_host.md #257

merged 2 commits into from May 5, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 4, 2022

Add fish because i like fish

Add fish
@ghost ghost changed the title Add fish Add tutorial for fish in execute_commands_on_host.md May 4, 2022
@@ -91,6 +93,28 @@ if [ -n "${ZSH_VERSION-}" ]; then
fi
```

### fish

Place this in a new fish config (such as `~/.config/fish/conf.d/cnf_handle.fish`:
Copy link
Contributor

Choose a reason for hiding this comment

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

The official fish documentation recommends that functions go in ~/.config/fish/functions and it should be the name of the function:

~/.config/fish/functions/fish_command_not_found.fish

I might reword the sentence as:

Place this snippet in a new fish configuration file (~/.config/fish/functions/fish_command_not_found.fish)

Copy link
Author

@ghost ghost May 5, 2022

Choose a reason for hiding this comment

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

sure, go ahead. can you modify my PR directly?

i was deciding whether I should change to ~/.config/fish/functions, but because my own testing was done with ~/.config/fish/conf.d I waited until I get access to my Linux laptop again so I can test.

```fish
function fish_command_not_found
# don't run if not in a container
if test ! -e /run/.containerenv -a ! -e /.dockerenv
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function is a little bit more readable if you:

  • reversed the logic of the container check
  • used more idiomatic fish, such as or and command -q

I think the exit 127 should be return 127 since it is a function.

function fish_command_not_found
    # "In a container" check
    if test -e /run/.containerenv; or test -e /.dockerenv
        if command -q flatpak-spawn
            flatpak-spawn --host $argv
        else if command -q host-exec
            host-exec "$argv"
        else
            return 127
        end
    else
        __fish_default_command_not_found_handler "$argv"
    end
end

Copy link
Author

@ghost ghost May 4, 2022

Choose a reason for hiding this comment

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

Does return 127 work? I don't have my Linux laptop nearby so I can't confirm.

I was about to change exit 127 to __fish_default_command_not_found_handler "$argv" because yesterday I found that exit 127 would cause shell to exit directly (Try this: Run invalid command within a distrobox that doesn't have flatpak-spawn or host-exec) and I only changed one of them (there're two exit 127 in the original function)

This function is mostly based on the original function for bash/zsh, I only modified it so it can run on fish, didn't notice the readability issue. I think you can modify my PR right?

grabbed a computer from my colleague
@ghost
Copy link
Author

ghost commented May 5, 2022

Re-request review button is broken.
@nathanchance

@nathanchance
Copy link
Contributor

Thanks! This looks fine to me from a fish perspective. I cannot test it though, as I don't use those commands. Hopefully I didn't give you the wrong impression but I am not the owner of this repo (@89luca89 is) so you'll need to wait for him to merge it.

@89luca89
Copy link
Owner

89luca89 commented May 5, 2022

@thjderjktyrjkt thanks a lot for the contribution!
and thanks @nathanchance for the help 😄

@89luca89 89luca89 merged commit 19aea0a into 89luca89:main May 5, 2022
@ghost ghost deleted the patch-1 branch June 4, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants