-
Notifications
You must be signed in to change notification settings - Fork 220
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
Support forward slash in Windows paths #933
Comments
Hey @miversen33 Is there any change this issue can be solved with (or something similar to) #1023 ? |
So the gist of this request is basically, whatever function actually resolves the path, it should accept this kind of pathing as well (though do we make it only accept this path if the os is windows? Idk). As an alternative, we could cheese it and have the state.path = state.path:gsub('/', '\\')
path_to_reveal = path_to_reveal:gsub('/', '\\')
path = path:gsub('/', '\\') to lua/neo-tree/sources/filesystem.init#L120 Thinking about it, I don't actually hate that idea. @cseickel thoughts on this? |
I think you have your slashes reversed. The request is to support forward slashes, unix style. Your suggestion is about escaping backslashes. I'm guessing there is a branch of code somewhere that interprets a path based on windows conventions if the os is windows which is not recognizing the use of a unix style path separator. I'd need more details on where the problem is before I can make any suggestions. I honestly never knew that windows would accept both unix style and windows style separators. It seems completely insane to me. What happens if you use both? Does backslash become an escape character or an alternate path separator? |
EDIT: Reading hard. You are correct, though my general suggestion still stands. Edited the above as well It seems a bit hacky, but unless I am mistaken, it shouldn't cause any issues with
So fun fact, apparently windows treats Because who needs consistency right? 🙃 |
If I understand correctly, in Both cases, Neovim's core team seems to prefer using But old vim functions prefer backslashes. |
100% @miversen33 I think you are on the right track, but the place to enforce that consistency is probably where the user input is first taken in, in the command module. Maybe in this function?
|
I noticed this inconsistency when digging into #1026 and I'm still not sure why there hasn't been much problem in neo-tree's code yet. 1. Opening file.Let's try to open
2. Look at
|
By doing this, we (as Neo-tree) are making the active decision that we will sanitize paths before passing them to the underlying source to handle. Not to say that's a bad thing, just a decision that I didn't feel comfortable making (which is why I moved the suggestion into the filesystem source instead of above in the command resolution, which is where I was initially looking). IMO, while taking this approach may be the best decision, it might be wise to take it as a 2 step approach. Resolving this direct issue (allowing forward slashes in the path), and then targeting path sanitation/fixing for My biggest concern is that one of these approaches is much smaller and easier to validate/verify than the other :) |
That's a good point. I didn't consider that someone could be on windows but still be passing an argument to a source that interacts with a *nix host such as a docker container or remote host. Given that, your suggestion of handling within the filesystem source is a good fix for this specific issue. Longer term we should handle paths in a consistent manner throughout the plugin and external sources. As @pysan3 pointed out, the issue is complicated and there are likely many edge cases waiting to be found within our codebase. I think we should drop all representations of paths as strings and replace them with plenary |
I appreciate you :) I'll slap together a PR for this this evening, the change is really small so it shouldn't take me too long to do and test.
IIRC Plenary Path's are a touch buggy but I haven't touched them since I was playing around with Telescope back in the day. I do think letting pathing be someone else's problem is a great solution. Properly handling paths is hard. |
I was trying to look into this a little bit, because it seemed insane and messy. surely enough, that's right, it is insane and messy. I'm totally for the sanitizing everything with the vim.fs.normalize approach, though IIRC there's some interesting problems and discussion around that subject in neovim recently.. ok here's the pr.. it looks like using this to sanitize might be ok now? neovim/neovim@8a7e335 |
Quick test with :lua print(vim.fs.normalize("C:\\Users/miver"))
C:/Users/miver I think we might be able to get away with using normalize in yes. It will take more testing to be sure it works as expected generally though :) |
Could you also stress test non ascii chars as well as emojis included in the path? We might also have to check how the git related codes work since Windows Git uses a weird encoding for non-ascii characters. This just came to my mind but would it be fixed if we execute Ref: #731 (comment) |
I will fetch dis. copy-la-pasta, here's the code fasta! --- Normalize a path to a standard format. A tilde (~) character at the
--- beginning of the path is expanded to the user's home directory and any
--- backslash (\\) characters are converted to forward slashes (/). Environment
--- variables are also expanded.
---
--- Examples:
--- <pre>lua
--- vim.fs.normalize('C:\\\\Users\\\\jdoe')
--- --> 'C:/Users/jdoe'
---
--- vim.fs.normalize('~/src/neovim')
--- --> '/home/jdoe/src/neovim'
---
--- vim.fs.normalize('$XDG_CONFIG_HOME/nvim/init.vim')
--- --> '/Users/jdoe/.config/nvim/init.vim'
--- </pre>
---
---@param path (string) Path to normalize
---@param opts table|nil Options:
--- - expand_env: boolean Expand environment variables (default: true)
---@return (string) Normalized path
function M.normalize(path, opts)
opts = opts or {}
vim.validate({
path = { path, { 'string' } },
expand_env = { opts.expand_env, { 'boolean' }, true },
})
if path:sub(1, 1) == '~' then
local home = vim.uv.os_homedir() or '~'
if home:sub(-1) == '\\' or home:sub(-1) == '/' then
home = home:sub(1, -2)
end
path = home .. path:sub(2)
end
if opts.expand_env == nil or opts.expand_env then
path = path:gsub('%$([%w_]+)', vim.uv.os_getenv)
end
return (path:gsub('\\', '/'):gsub('/+', '/'):gsub('(.)/$', '%1'))
end
yeah, it's simply transforming it, not making sure it exists. It does do some expansion of env vars though, so that's cool. I don't know if that helps, but it's cool. shocking about the emoji thing.. 🤷♂️ I'm envisioning a dystopian future full of filesystems that look like we will soon have to support the .🦅 file extension.. you have been warned. |
Thank you @WillEhrendreich I appreciate you. So this is taking longer than expected as I am unsure the best thing to do here. @cseickel, since we can verify that I feel rather comfortable with that as we can see the source for the function isn't doing anything potentially nefarious with the paths being passed in (and thus we aren't validating them, just normalizing them for subsequent sources. Which IMO is good). |
I would leave this up to you @miversen33, because netman is the most likely extension to have a problem with us changing the paths so you would be the best person to make decisions on that. Also, I don't have a Windows computer that I can use for testing so I can't really verify anything. |
PR #1051 has been merged into main. I am going to close this issue out, though if my resolution did not fix this, please either tag me on this so it can be reopened, or open a new issue :) |
Windows accepts forward slashes as path component separators in most of their API's (and in explorer.exe as well as cmd.exe). It would be nice if neo-tree.nvim did the same.
This does not work on Windows:
Looks like it tries to open a non-existant path.
This does work:
And the contents of
bar
is shown.The text was updated successfully, but these errors were encountered: