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

Support Windows UNC path #245

Closed
wants to merge 3 commits into from
Closed

Conversation

Milly
Copy link
Contributor

@Milly Milly commented Oct 11, 2020

Unfortunately neovim does not support UNC path.

in neovim 0.4.4

  • getftype('\\localhost\C$\') => 'dir' 👌
  • glob('\\localhost\C$\*') => [] 😢
  • getcompletion('\\localhost\C$\', 'file') => [] 😢

Copy link
Owner

@lambdalisue lambdalisue left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. But I'm sorry, I don't think supporting the Windows UNC path has worth introducing this much complexity to the existing code. Please consider making a new fern scheme plugin to support it (like fern:///unc://{host}/{path}).

@@ -12,26 +12,28 @@ function! fern#fri#new(partial) abort
endfunction

function! fern#fri#parse(expr) abort
Copy link
Owner

Choose a reason for hiding this comment

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

I don't get why you need to apply changes to fern#fri#parse().
Supporting the Windows UNC path should not touch FRI itself or let me know the reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix authority and path parsing.

Changed result of 'foo://bar/' below.

""" Old
echo fern#fri#parse('foo://bar/qux')
" => {'query': {}, 'authority': 'bar', 'fragment': '', 'scheme': 'foo', 'path': 'qux'}
echo fern#fri#parse('foo://bar/')
" => {'query': {}, 'authority': '', 'fragment': '', 'scheme': 'foo', 'path': 'bar'}
echo fern#fri#parse('foo://bar')
" => {'query': {}, 'authority': '', 'fragment': '', 'scheme': 'foo', 'path': 'bar'}

""" New
echo fern#fri#parse('foo://bar/qux')
" => {'query': {}, 'authority': 'bar', 'fragment': '', 'scheme': 'foo', 'path': 'qux'}
echo fern#fri#parse('foo://bar/')
" => {'query': {}, 'authority': 'bar', 'fragment': '', 'scheme': 'foo', 'path': ''}
echo fern#fri#parse('foo://bar')
" => {'query': {}, 'authority': '', 'fragment': '', 'scheme': 'foo', 'path': 'bar'}

Copy link
Owner

Choose a reason for hiding this comment

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

I see. So this is kinda bug fix, right? It would be nice if you separate it to a different PR.

@@ -30,6 +30,14 @@ function! fern#internal#filepath#is_absolute(path) abort
\ : s:is_absolute_unix(a:path)
endfunction

function! fern#internal#filepath#is_unc_compat() abort
return g:fern#internal#filepath#is_windows
Copy link
Owner

Choose a reason for hiding this comment

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

If Neovim 0.4.4 does not support the UNC path, we should check it here, right?

Additionally, it would be nice to make an issue on the Neovim side and put the issue URL here to explain why we need to exclude Neovim.

endfunction

function! fern#internal#filepath#is_uncpath(path) abort
return g:fern#internal#filepath#is_windows && s:is_uncpath(a:path)
Copy link
Owner

Choose a reason for hiding this comment

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

We should NOT check g:fern#internal#filepath#is_windows here while the function purpose is to check if the path is a UNC path or not.

function! s:is_absolute_windows(path) abort
return a:path ==# '' || a:path[:2] =~# '^\w:\\$'
return a:path ==# '' || a:path[:2] =~# '^\w:\\$' || s:is_uncpath(a:path)
Copy link
Owner

Choose a reason for hiding this comment

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

I guess it's not correct. It just checks if the path is a UNC path and it does not check if the path is an absolute UNC path, right?

let path = fern#internal#filepath#to_slash(a:node._path)
let parent = fern#internal#path#dirname(path)
let parent = fern#internal#filepath#from_slash(parent)
if s:is_windows && fern#internal#filepath#is_uncpath(a:node._path)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should use fern#internal#filepath#is_uncpath_compat()

if s:is_windows && path ==# ''
return s:windows_drive_root
elseif s:is_windows && fern#internal#filepath#is_uncpath(path)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should use fern#internal#filepath#is_uncpath_compat()

@@ -1,17 +1,18 @@
function! fern#internal#command#do#command(mods, fargs) abort
function! fern#internal#command#do#command(mods, qargs) abort
Copy link
Owner

Choose a reason for hiding this comment

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

The function interface must not be changed while the function itself is OK.

@@ -7,12 +7,12 @@ let g:fern_loaded = 1 " Obsolete: For backward compatibility
command! -bar -nargs=*
\ -complete=customlist,fern#internal#command#fern#complete
\ Fern
\ call fern#internal#command#fern#command(<q-mods>, [<f-args>])
\ call fern#internal#command#fern#command(<q-mods>, <q-args>)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm planning to change the entire behavior of args handling because of #246 thus please don't touch these in this PR.

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.

2 participants