linux-cultist / venv-selector.nvim

Allows selection of python virtual environment from within neovim
MIT License
388 stars 41 forks source link

fd command not working in Ubuntu distro #6

Closed Stewart86 closed 1 year ago

Stewart86 commented 1 year ago

Context

OS: Windows 11 running WSL2 Ubuntu 22.04 Default Shell set with chsh: Fish Neovim version: 0.8 Plugin manager: Lazy Configuration used: according to README.md guide for Lazy requirements installed: fd: yes (using apt install fd-find) telescope: yes

Expected behaviour

Using mapped key (\<leader>vs) should show telescope with a list of virtualenvs to pick from.

Actual behvaiour

after keying <leader>vs nothing happened.

Investigate

After some debugging, realised that vim.loop.spawn() is using the sh shell to execute the fd command. As I am using Ubuntu, according to fd guide there is a conflict with another binary in the distro, so alias was suggested to be use. As a result, I have set alias for bash shell and fish shell. But vime.loop.spwan() is not using either one.

turn out I changed the command from fd to fdfind and it works as expected.

Proposal

Allow a config variable to change fd to fdfind or find depends on user preference.

linux-cultist commented 1 year ago

Ah thank you for taking the time to understand the problem and suggest a good fix. :) Had no idea about this being the case on Ubuntu. I will add a quick option for setting the fd variable to some other name.

linux-cultist commented 1 year ago

If you update the plugin now, you can set the option "fd_binary_name" to the name of fd you have on your system. I added it to the README also but as an example:

require("venv-selector").setup({
    fd_binary_name = "fdfind"
})

VenvSelect uses and requires fd though and wont work with the ordinary linux find command. Its just too slow to be convenient.

linux-cultist commented 1 year ago

Hopefully this solution works for you - it should! Otherwise feel free to reopen the issue. Thank you for reporting this!

Stewart86 commented 1 year ago

Yes! Perfect! it works now. Thanks for being so quick to fix this. Except, there are a couple of improvement I could also suggest. If i have time later today, I might raise a PR for it. Or if you got around to it, that would be great!

  1. expose a venv name for statusline display so we know which python interpreter we are using any time by looking at the statusline. Maybe something like venv_name = (poetry) my_project_2j8w2a23

  2. instead of printing use notify API to notify the activation of the venv

  3. when a venv is set, remember the setting, so the next time vim is loaded, it will default to that saved venv instead of needing to activate again manually

linux-cultist commented 1 year ago

Thanks for the kind words! :)

For number one, there are two functions you can call. I added them yesterday so they are new, but there is info in the readme.

Number 2 - yes I've been thinking this also. You mean the notify plugin right? I could try to load that one and use it if it exists. I don't like the printing very much either and would prefer a better way.

Number 3 - This could easily be implemented and put behind an option. Maybe even better if it's tied to a specific workspace, so it would remember per workspace?

If you want to, make PR's, always nice to get help :)

Stewart86 commented 1 year ago

You are welcome!

  1. are you saying the path variable that is exposed? If that is the case, I am actually looking at a somewhat shorter venv name. I did some modification to your code. This is how it looks. let me know if you are okay with this. But of cause I did some beautify to my statusline to get the python logo. In any case, you might want to keep the config simple then I guess moving this logic downstream to users own preferences is fine to me too.

image

  1. Actually neovim have a native API vim.notify() that is very easy to implement. [reference](https://neovim.io/doc/user/lua.html#vim.notify())

  2. I like this idea. but I am not sure how I can do this.

I have created a branch for item 1 and 2. if you are okay with it I will do a PR.

But I have one more issue. I have a rather unusual project that requires me to configure pyright using the pyproject.toml file to set different executionEnvironments for different folders in my project. However it broke when I use this plugin. After investigation I realised that if i comment out M.set_pythonpath(venv_python) everything work fine again. even with a simple project without additional pyright configuration in pyproject.toml it works fine too. That makes me wonder if it is necessary to add pythonPath into pyright. Maybe you can enlighten me on this.

for you info, this is my pyright setup in lspconfig

return function(opts)
  opts.root_dir = require("lspconfig.util").root_pattern { ".git", "pyproject.toml" }
  opts.settings = {
    python = {
      analysis = {
        typeCheckMode = "strict",
        autoSearchPaths = true,
        useLibraryCodeForTypes = true,
        exclude = { "**/tests" },
      },
    },
  }
  return opts
end
linux-cultist commented 1 year ago
  1. Both the path of the venv folder and the path to the python can be retrived by calling these functions:

require("venv-selector").get_active_path() -- Gives path to the python executable inside the activated virtual environment require("venv-selector").get_active_venv() -- Gives path to the activated virtual environment folder

I think its better to give the user the path without any formatting, and then allow the user to format the information how he/she wants in the statusbar. If we add ("poetry") here in the output, users who dont want that will have to try and scrub that information away, and also the function wont work with other plugins that just expects a path. :)

So for your use case - call these functions from your statusbar, and then format the output how you want it to look. :) There are vim functions for shortening a path for example that you can use in your status bar code.

  1. Cool, didnt actually know this. I guess also if we use vim.notify, then if someone has the nvim notify plugin, it will automatically use the plugin. So yeah, lets use vim.notify to show the "venv activated" message.

  2. Interesting... its necessary to add pythonPath for ordinary venvs to be activated by the plugin. I think with your config above, lspconfig would always pick the root dir where it finds a git folder or a pyproject.toml folder, so it doesnt really care what you pick with VenvSelect. But I could be wrong, need to do more testing on this for sure. :) I think thats also why it works for you when VenvSelect doesnt attempt to activate the venv - because it will already be activated by your config above.

But I do feel like I could be wrong about some of these things and I could use some more testing to understand more. :)

Stewart86 commented 1 year ago
  1. got it. You have a point.
  2. I have made a PR for this
  3. I did some experiment, by removing the root dir my code works fine too with M.set_pythonpath(venv_python) commented out. But break after I include the function back haha.

I also checked lspconfig repo on pyright portion. it did not mentioned anything about before_init like what you did in your code. Maybe I just don't have much in depth knowledge on the lsp side of things.

linux-cultist commented 1 year ago

I'm also not that knowledgeable about the lsp stuff, need to investigate more going forward. :)

nikolaevigor commented 1 year ago

Allow a config variable to change fd to fdfind or find depends on user preference.

May I suggest to add some indication if fd binary not found. Also have fdfind instead of fd, and it took me some debugging to understand why nothing happens on :VenvSelect. So some message with error would be great

linux-cultist commented 1 year ago

Yes you are correct :) There is an issue for this already and I wanted to detect if fdfind is available on the system and use that one if it is. And otherwise throw an error message.

So it's coming, just had a bit limited time :)