WIP: fix working in directories with spaces in path #781
Conversation
|
I have signed the CLA! |
| @@ -67,7 +67,7 @@ def run | |||
| return if @formatter == "none" | |||
|
|
|||
| # Don't try to format files outside the current working directory | |||
| return unless @uri.sub("file://", "").start_with?(Dir.pwd) | |||
| return unless URI(@uri).path&.start_with?(T.must(WORKSPACE_URI.path)) | |||
There was a problem hiding this comment.
Instead of handling this specific case in formatting, I believe the way to go is to standardize URI handling in the entire LSP.
That would mean changing Document#uri to be a URI instance and making sure we're no longer assuming it to be file:// anywhere.
There was a problem hiding this comment.
Yes, that might be a better way to go, but that is out of my perceived capability for now.
Aka: I don't think I could do this at this time. Especially because I am not familiar with the project structure and with sorbet.
| @@ -6,7 +6,7 @@ module RubyLsp | |||
| VOID = T.let(Object.new.freeze, Object) | |||
|
|
|||
| # This freeze is not redundant since the interpolated string is mutable | |||
| WORKSPACE_URI = T.let(URI("file://#{Dir.pwd}".freeze), URI::Generic) | |||
| WORKSPACE_URI = T.let(URI(URI::Parser.new.escape("file://#{Dir.pwd}".freeze)), URI::Generic) | |||
There was a problem hiding this comment.
I have a question regarding this one: does the editor send spaces encoded or unencoded? That is, do we receive file:///something otherthing/blah.rb or file:///something%20otherthing/blah.rb?
And does URI() not handle does already? If I'm not mistaken URI() invokes the parser, so I'm surprised it doesn't handle the spaces.
There was a problem hiding this comment.
In RubyLsp::Server we do receive the URI encoded with percent encodes: file:///something%20otherthing/blah.rb
This seems to also work correctly with diacritics and emoticons: file:///the/code/app/%C3%BCber%20%F0%9F%A4%A9/test.rb decodes to: file:///the/code/app/über 🤩/test.rb.
Apparently URI does not handle this correctly. I am not 100% sure if this is intentional, but URI does not know this is a file path, and URI does not know what it should encode. It might lead to double encodes if we hand it an already encoded path.
I will try to lookup inside the URI gem, if there is a hint how this is intended to be used.
There was a problem hiding this comment.
The URI gem does not have a better answer to this. I created ruby/uri#87 over there.
this fixes starting ruby-lsp in directories with space in the path name
the condition did not take special characters in Dir.pwd into account. Use the same condition as used in RubyLsp::Requests::Diagnostics
|
I just opened Shopify/vscode-ruby-lsp#716 on the extension itself, because that apparently broke space-paths, too, since I last worked on this. |
tonklon
force-pushed
the
feature/fix-spaces
branch
from
056ca8a
to
931e7eb
Compare
|
I added some tests, I hope this makes sense. |
|
Thank you very much for the PR, but I do think it's worth for us to start handling URIs properly and stop the mess of representing them as strings from spreading further. I put up #848 to fully change to URI objects and it should address the issues with spaces/emojis in paths. |
|
@vinistock The update with use the URIs everywhere makes a lot of sense. Thanks for that. But this issue is not fixed by just that change. It is still reproducible using the master branch. The code The consequent use of URI helps a lot so we don't have to encode paths at all usage sites, but at this place we really need to do something. I think the URI gem is missing out an important method here to help create a file: URI from a path. But remember URI() will just parse an already valid URI. This is the API contract. And I will create a new PR based on the work you already did. |
This addresses #709 and allows ruby-lsp to work in directories with spaces in the path name.
The handling of paths in ruby-lsp is not really consistent, sometimes URIs are used sometimes, strings. Sometimes converting between the two is correct sometimes not.
This PR currently solves the problem I am facing in my setup, but I like to further improve this by:
Feedback is already welcome, if there's something to improve.