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

Unknown command: mcfly_key_bindings on fish shell #129

Closed
domoritz opened this issue Mar 5, 2021 · 12 comments · Fixed by #137
Closed

Unknown command: mcfly_key_bindings on fish shell #129

domoritz opened this issue Mar 5, 2021 · 12 comments · Fixed by #137
Labels
help wanted Extra attention is needed

Comments

@domoritz
Copy link
Contributor

domoritz commented Mar 5, 2021

I installed McFly via home-brew. I have version McFly 0.5.4. I also added

# mcfly

mcfly init fish | source
mcfly_key_bindings

set -gx MCFLY_LIGHT true
set -gx MCFLY_FUZZY true

to the end of my config.fish file. I have fish version 3.2.0.

Even when I run mcfly init fish | source in my terminal, mcfly_key_bindings is still not defined.

@cantino
Copy link
Owner

cantino commented Mar 5, 2021

Thanks for the report @domoritz. @b3nj5m1n, any ideas?

@domoritz domoritz changed the title Unknown command: mcfly_key_bindings Unknown command: mcfly_key_bindings on fish shell Mar 6, 2021
@domoritz
Copy link
Contributor Author

domoritz commented Mar 6, 2021

Does the new version work for anyone on fish shell?

I suppose the issue is that source isn't is-interactive. Also, it seems problematic to call mcfly_key_bindings in non interactive scenarios.

I would probably change mcfly init fish to not check for interactive and ask people to add

if status is-interactive
	mcfly init fish | source
	mcfly_key_bindings
end

or figure out a way to add to call mcfly_key_bindings inside mcfly init fish.

@cantino
Copy link
Owner

cantino commented Mar 11, 2021

Sorry, I don't use fish shell. I think this is due to the changes made by @b3nj5m1n in #119, but I'm not sure. Does adding mcfly_key_bindings after like 77 of https://github.com/cantino/mcfly/blob/master/mcfly.fish help?

@cantino
Copy link
Owner

cantino commented Mar 11, 2021

@tjkirch, any help on this would also be appreciated!

@tjkirch
Copy link
Contributor

tjkirch commented Mar 11, 2021

I'd personally stick with the approach from before 23ea43b. It's closer to standard and doesn't require any extra checks in the user's config.

@cantino
Copy link
Owner

cantino commented Mar 13, 2021

I don't understand why there would be any difference between source "/usr/local/opt/mcfly/mcfly.fish" and mcfly init fish | source.

@domoritz
Copy link
Contributor Author

I think the problem is https://github.com/cantino/mcfly/blob/master/mcfly.fish#L4.

If I create a file test.fish with test -t 0; or echo "no" and then cat test.fish | source I get no.

Not sure what this really ensures and why we don't use https://fishshell.com/docs/current/cmds/isatty.html.

If I remove the line, it works. I also think it makes sense to add mcfly_key_bindings to mcfly init fish as recommended in #129 (comment) since otherwise we would call mcfly_key_bindings even in a non-interactive shell.

@cantino
Copy link
Owner

cantino commented Mar 14, 2021

Would you be open to making a pull request that switches to isatty and pulls mcfly_key_bindings inside the file? I don't use fish so have somewhat low confidence that I'll get it right.

@cantino cantino added the help wanted Extra attention is needed label Mar 14, 2021
@domoritz
Copy link
Contributor Author

isatty doesn't work either unfortunately.

$ cat test.fish
isatty stdin; or echo "no"

$ cat test.fish | source
no

$ source test.fish

$ isatty stdin; or echo "no"

I don't know enough about source and isatty is say whether this is a bug in fish (like fish-shell/fish-shell#2477) or whether this is expected behavior.

@domoritz
Copy link
Contributor Author

@b3nj5m1n do you have a suggestion for how to fix McFly on fish? Otherwise, I will suggest to remove the isatty check.

@b3nj5m1n
Copy link
Contributor

No, sorry but I don't use fish. I'd say try your approach.

domoritz added a commit to domoritz/mcfly that referenced this issue Apr 1, 2021
@domoritz
Copy link
Contributor Author

domoritz commented Apr 1, 2021

I sent a pull request in #137. It works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants