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

Rehab the plugin #5

Merged
merged 4 commits into from
Feb 7, 2020
Merged

Rehab the plugin #5

merged 4 commits into from
Feb 7, 2020

Conversation

scorphus
Copy link
Member

@scorphus scorphus commented Feb 5, 2018

@Lukas0907
Copy link

Lukas0907 commented Feb 25, 2018

Hi,

I think this can be solved with pyenv more easily. Running "pyenv init -" from fish already prints the following stub:

set -gx PATH '/home/lukas/.pyenv/shims' $PATH
set -gx PYENV_SHELL fish
source '/home/lukas/.pyenv/libexec/../completions/pyenv.fish'
command pyenv rehash 2>/dev/null
function pyenv
  set command $argv[1]
  set -e argv[1]

  switch "$command"
  case activate deactivate rehash shell
    source (pyenv "sh-$command" $argv|psub)
  case '*'
    command pyenv "$command" $argv
  end
end

So shipping the pyenv function and the completions in this plugin is not necessary.

To make plugin-pyenv more useful, I think it would be enough to source the output of pyenv init - any maybe the output of the virtualenv-init subcommand (see also #1).

@BarbzYHOOL
Copy link

BarbzYHOOL commented Dec 4, 2018

It works when I install the plugin the first time (with fundle install) but then after I open a new terminal, pyenv command never exists so basically the $PATH is not set

It's the first line of the init.fish that fails

I even tried with set PYENV_ROOT $HOME/.pyenv but it doesn't set it o_0

@scorphus
Copy link
Member Author

scorphus commented Dec 4, 2018

@Lukas0907 we should try our best to keep the plugin as lazy as possible. Imagine how long a fish user would need to wait for the command prompt if every single installed plugin would execute one or more commands on init.

@BarbzYHOOL thanks for chiming in and calling for action here. Why do you need the PYENV_ROOT variable for?

@BarbzYHOOL
Copy link

because the plugin is not working at all for me, nothing in init.fish is read o_0

And look at the latest file it's here https://github.com/oh-my-fish/plugin-pyenv/pull/5/files

@scorphus
Copy link
Member Author

scorphus commented Dec 5, 2018

@BarbzYHOOL do you know how to test that PR? In the meantime, I'll make sure it works on some Docker images. The I'll merge that PR.

@BarbzYHOOL
Copy link

I tested the PR and even merged it in my own fork and I'm using it at the moment (with the bugs I reported above)

The only difference is that I use "fundle" instead of fisher (I opened an issue in fundle but I checked other plugins I use, and they have the same exact init.fish and they work fine, in top of that if I copy paste the content of plugin-pyenv/init.fish into another-plugin/init.fish it works fine o_0)

@scorphus
Copy link
Member Author

scorphus commented Dec 6, 2018

@BarbzYHOOL I've just tested the rehab branch in both Oh My Fish and Fundle. Both fresh installs on two separate Fish Shell 2.7.1 containers. I can share both with you, if you'd like.

Regarding Fundle, I just followed instructions in https://github.com/danhper/fundle#installation and https://github.com/danhper/fundle#usage.

Can you test it that way too?

@BarbzYHOOL
Copy link

All my other plugins work fine, I use fundle for months

and I checked, my plugin-pyenv repo (in fundle/) has the right commits:

4602a38 2018-11-11 Barbz (HEAD, origin/master, origin/HEAD) Merge pull request #2 from BarbzYHOOL/rehab 
287d8ef 2018-11-11 Barbz Merge branch 'master' into rehab 
887e2ae 2018-11-11 Barbz Add missing commands from official pyenv install 
2e00b6c 2018-01-31 Pablo Santiago Blum de Aguiar (origin/rehab) Improve README.md a bit 
53dad28 2017-05-26 Gustavo Pantuza Readme: Create base documentation of how to use plugin 

So it's working for you on fundle ??

Did you exit your terminal, and open another? because as I said, the first installation works, then after it never works again

@scorphus
Copy link
Member Author

scorphus commented Dec 6, 2018

Yep. I made it work. Then made it stop working by commenting contents of init.fish. Then make it work again by uncommenting them.

So, as soon as I can, I'll share with you the Docker image I used and you can check how it goes for you.

@BarbzYHOOL
Copy link

BarbzYHOOL commented Dec 6, 2018

I don't use docker so don't send me the docker image (but thank you for proposition). hmmm there must be something wrong with me but I have no idea what :'(
I haven't made weird stuff like symlink etc

I have no idea what to do :(

@scorphus
Copy link
Member Author

scorphus commented Dec 6, 2018

Let's see if I can help you. What Fish version are you running? Can you share the content of config.fish and also the content of plugin-pyenv's init.fish?

@BarbzYHOOL
Copy link

BarbzYHOOL commented Dec 7, 2018

omg I use two config file, the normal one and one in /etc/fish. I just posted and saw that there is no fundle init in the first config file.

so it only affected my plugin-pyenv because I have only one plugin in that conf.fish

So it's solved thanks to you @scorphus haha

init.fish Outdated
@@ -1,3 +1,4 @@
set -q PYENV_ROOT; or set -l PYENV_ROOT $HOME/.pyenv

set PATH $PYENV_ROOT/shims $PYENV_ROOT/bin $PATH
set PATH $PYENV_ROOT/shims $PYENV_ROOT/bin $PATH ^ /dev/null
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change this line as ^ operator is deprecated.

@derekstavis
Copy link
Member

@scorphus any chance this is still good to merged?

@scorphus
Copy link
Member Author

Oh, man. I keep forgetting this PR. I'm still using this branch with a small unstaged change on top. So far it's been working fine. Did you by any chance manage to test it?

@BarbzYHOOL
Copy link

i think I tested it from the msg I put above

@scorphus
Copy link
Member Author

So, @derekstavis, once it has your approval (or approval from any of the core devs) as usual, I'll pull master.

@derekstavis
Copy link
Member

Done! Thanks for following up here!

@scorphus scorphus merged commit 55670a1 into master Feb 7, 2020
@scorphus scorphus deleted the rehab branch February 7, 2020 18:52
@scorphus
Copy link
Member Author

scorphus commented Feb 7, 2020

Thanks for contributing/testing/reviewing, @BarbzYHOOL, @brunobbbs, @pantuza, and @derekstavis! 🐠🐚

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.

6 participants