supermaven-inc / supermaven-nvim

The official Neovim plugin for Supermaven
https://supermaven.com/
MIT License
278 stars 16 forks source link

FIX: Windows OS web fetching #33

Closed 3dfactor closed 1 month ago

3dfactor commented 1 month ago

It was failing to download anything for me on Windows version of nvim. After spreading vim.fn.system it works - must be something to do with how spaces/quotes are handled in Windows OS.

AlejandroSuero commented 1 month ago

@3dfactor @sm-victorw I don't know if this should be added to this PR but we could use curl if installed in Windows too, like for example in glow.nvim, in my Windows machine it downloads with no problem having curl installed.

if vim.fn.executable("curl") == 1 then
-- curl cmd
elseif platform == "windows" then
-- pwsh cmd
else
-- error: couldn't fetch binary
end

[!NOTE]

It also can be used vim.fn.exepath("curl") == ""

3dfactor commented 1 month ago

@AlejandroSuero I don't know if it should be in this PR, however I've tried using curl, since I have it on my Windows side of things and it still needed destructuring to work properly. I've looked at your given example in glow.nvim and apparently they also use destructured approach:

local download_command = { "curl", "-sL", "-o", "glow.tar.gz", release_url }
...
vim.fn.jobstart(download_command, callbacks)

So maybe it should be destructured by default regardless of OS consuming it to avoid possible caveats with spaces/quotes handling on different environments?

AlejandroSuero commented 1 month ago

@3dfactor yes, I forgot to mention that cause I'm used to do it that way when using vim.fn.system or os.execute.

3dfactor commented 1 month ago

Well, I could add both:

However, that would change the PR from being Windows fix to something else. Should I close it and add new PR or edit on top of this one?

sm-victorw commented 1 month ago

It looks like glow.nvim only ever uses curl, and does not attempt to use any powershell commands. I'm worried it could be redundant to have both, introducing unneeded complexity, and making things a bit more error prone. But if the behavior of curl on windows is generally more consistent and predictable, then it would make sense to prefer using it. I think it would be simpler for that case to be its own PR

3dfactor commented 1 month ago

It looks like glow.nvim only ever uses curl, and does not attempt to use any powershell commands. I'm worried it could be redundant to have both, introducing unneeded complexity, and making things a bit more error prone. But if the behavior of curl on windows is generally more consistent and predictable, then it would make sense to prefer using it. I think it would be simpler for that case to be its own PR

As far as I can tell, curl has been a part of Windows OS since 2017, so it should be safe to use. https://curl.se/windows/microsoft.html