Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Convert lowercase drive letters to uppercase #953

Merged
merged 7 commits into from
Dec 25, 2020
Merged

Convert lowercase drive letters to uppercase #953

merged 7 commits into from
Dec 25, 2020

Conversation

bamchoh
Copy link
Contributor

@bamchoh bamchoh commented Dec 19, 2020

I faced a problem that error diagnostics message does not shown in golang source code. In a result of my investigation, this problem occurs when I change the current directory with lowercase driveletter like cd c:\dev\go\test001. When cd C:\dev\go\test001 is no problem. To avoid this problem, we need to convert driver letter to uppercase in lsp#utils#path_to_uri function.

I faced a problem that error diagnostics message does not shown in golang source code. In a result of my investigation, this problem occurs when I change the current directory with lowercase driveletter like `cd c:\dev\go\test001`. When `cd C:\dev\go\test001` is no problem. To avoid this problem, we need to convert driver letter to uppercase in lsp#utils#path_to_uri function.
@prabirshrestha
Copy link
Owner

Can you add tests for utils.

Path codepath is quite fragile so having unit tests would help with regressions.

@bamchoh
Copy link
Contributor Author

bamchoh commented Dec 22, 2020

Thank you for your advice. I added a test.

@prabirshrestha
Copy link
Owner

I need to think about this since it can cause lot of issues. If we are only having issues with diagnostics can we instead fix the diagnostics?

function! s:get_diagnostics(uri) abort
if has_key(s:diagnostics, a:uri)
return [1, s:diagnostics[a:uri]]
else
if s:is_win
" vim in windows always uses upper case for drive letter, so use lowercase in case lang server uses lowercase
" https://github.com/theia-ide/typescript-language-server/issues/23
let l:uri = substitute(a:uri, '^' . a:uri[:8], tolower(a:uri[:8]), '')
if has_key(s:diagnostics, l:uri)
return [1, s:diagnostics[l:uri]]
endif
endif
endif
return [0, {}]
endfunction

There are tools like driveup which forces uppercase drive letter but I don't expect them this to be used all the time.

@prabirshrestha
Copy link
Owner

some more discussions about this at microsoft/language-server-protocol#1019

@bamchoh
Copy link
Contributor Author

bamchoh commented Dec 24, 2020

Thank you for explanation why we should not modify utils.vim for lowercase driver letter. I understood it.
Also I understood that you had already code for lowercase driver letter because a typescript language server treat driver letter as lowercase.
On the other hands, it seems that gopls treat drive letter as uppercase internally.
https://github.com/golang/tools/blob/bce87a7896ccdbaeca70664555ea4d0e6cb1444b/internal/span/uri.go#L141
Hence, I faced this issue.
To treat driver letter for both cases properly, I revise diagnostics.vim so that modify the uri which is returned from the server on "textDocument/publishDiagnostics" to lowercase everytime.

I'm sorry that I did not add test codes for this modification because I don't know how to add them.

@prabirshrestha
Copy link
Owner

Here is what i would do.

  • Create a function that normalizes a uri in utils so it can be tested independently. Normalized function should be a noop in non-windows environment.
  • Then in diagnostics.vim call the normalized function

Having the normalized function would allow us reuse it for other features.

To run tests I would add a vim-themis as a vim plugin. Plug 'thinca/vim-themis' then add vim-themis\bin in your path. Then go to vim-lsp plugin folder and type themis.bat. It should run all the tests. if you want to only run your tests you can use --target pattern i.e. themis.bat --target uri. Someday I should most likely have doc/vim-lsp-dev.txt.

@prabirshrestha
Copy link
Owner

Looking good. Added a nit comment. Can you fix it while I try this on my windows machine to see if it works on typescript language server.

Copy link
Owner

@prabirshrestha prabirshrestha left a comment

Choose a reason for hiding this comment

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

I tried it on windows with rust-analzyer and typescript-langauge-server and works.

I have added few more comments before I merge.

Thanks for the PR.

autoload/lsp/ui/vim/diagnostics.vim Outdated Show resolved Hide resolved
@bamchoh
Copy link
Contributor Author

bamchoh commented Dec 25, 2020

Thank you for your code review. I added a comment. But I'm not sure whether or not it is correct English because I'm not native English speaker.

@@ -78,6 +78,9 @@ endif

if has('win32') || has('win64')
function! lsp#utils#normalize_uri(uri) abort
" Now, it is discussing about normalization of uri.
Copy link
Owner

@prabirshrestha prabirshrestha Dec 25, 2020

Choose a reason for hiding this comment

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

Change to the following.

" Refer to https://github.com/microsoft/language-server-protocol/pull/1019 on normalization of urls.
" TODO: after the discussion is settled, modify this function. 

@prabirshrestha
Copy link
Owner

No worries. I added comment for the comment. Then should be good to merge.

@bamchoh
Copy link
Contributor Author

bamchoh commented Dec 25, 2020

Thank you very much for your kindly review. I revised a comment as per as your suggestion.

@prabirshrestha prabirshrestha merged commit 5f23ecd into prabirshrestha:master Dec 25, 2020
@prabirshrestha
Copy link
Owner

Merged. Thanks for the PR and your patience.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants