-
Notifications
You must be signed in to change notification settings - Fork 154
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
WIP: fix working in directories with spaces in path #781
Conversation
I have signed the CLA! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests would be great. LGTM so far.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.