-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Convert LSP URLs into custom URIs #9652
Conversation
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'm fully on board with this direction as an iteration on the one i proposed.
(note: i approve of this PR, but think it a bit too mischievous to approve a PR with some of my own commits in it)
helix-term/src/application.rs
Outdated
let language_server = language_server!(); | ||
if !language_server.is_initialized() { | ||
log::error!("Discarding publishDiagnostic notification sent by an uninitialized server: {}", language_server.name()); | ||
return; | ||
} | ||
// have to inline the function because of borrow checking... | ||
let doc = self.editor.documents.values_mut() | ||
.find(|doc| doc.path().map(|p| p == &path).unwrap_or(false)) | ||
.find(|doc| doc.path().map(|p| p == path).unwrap_or(false)) |
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.
out of scope for this PR (of course) but it's making more and more sense to switch to indexing documents by URI...
610ae26
to
52a2033
Compare
Thanks for the feedback @soqb! I've left most of the path conversions as |
52a2033
to
5419101
Compare
Shouldn't this be provided by Unaware of this pull request, I had raised an issue on |
It would definitely be convenient if lsp_types took care of normalization. We would still want a Helix-specific URI though for future expansion. Calling it a spec violation seems like editorializing though. The spec only says that clients and servers should be careful to handle different encodings consistently: it's convenient if that's taken care of at deserialization time but not mandatory. Converting all URIs to |
Yes, I think we will need something helix specific sooner or later.
On a second re-read, I agree it is not exactly a violation of the spec. I had mistakenly assumed that this is what it meant while looking at the reference implementation. Sorry for that. So it seems we can't rely on |
this branch has a conflict otherwise lgtm |
Co-authored-by: soqb <cb.setho@gmail.com>
Co-authored-by: soqb <cb.setho@gmail.com>
5419101
to
6de3b4d
Compare
This introduces a custom URI type for core meant to be extended later if we support other schemes. For now it's just a wrapper over a
PathBuf
. We use this newUri
type to firewalllsp::Url
so that:+
or Windows drive letters)csharp:/metadata/foo/bar/Baz.cs
URLs)Closes #2109
Closes #3267
Closes #7367