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

Vendor lsp-types #11355

Merged
merged 7 commits into from
Jul 31, 2024
Merged

Vendor lsp-types #11355

merged 7 commits into from
Jul 31, 2024

Conversation

the-mikedavis
Copy link
Member

This pulls in the lsp-types crate almost as-is from the 0.95.1 release. The only change is the crate name so far. The plan is to replace the URL type used throughout the crate with a String or Box<str> which we can parse on our end and convert to a helix-core::Uri. That is left to future work - this PR just vendors the crate.

@the-mikedavis the-mikedavis added the A-dependencies Area: Dependency label Jul 28, 2024
@the-mikedavis the-mikedavis marked this pull request as draft July 28, 2024 13:41
@pascalkuthe
Copy link
Member

There are some parts of LSP we don't use/will never support. I wonder if it's wroth stripping down the types a bit to only the functionality we actually use to reduce compile time.

I think I would also like to experiment eith improving the API in places in the future. There are some instances where I remeber wishing lsp-types exposing a different API.

@pascalkuthe
Copy link
Member

The plan is to replace the URL type used throughout the crate with a String or Box

I think a newtpye wrapper would be convenient (or at least a type alias). That makes it easy to model URLs with a different type in the future (just replace all uses of that type) and is a bit more self-documenting

helix-lsp-types/Cargo.toml Outdated Show resolved Hide resolved
}

#[derive(Debug, Eq, PartialEq, Hash, PartialOrd, Clone, Deserialize, Serialize)]
pub struct CodeActionKind(Cow<'static, str>);
Copy link
Member

@pascalkuthe pascalkuthe Jul 28, 2024

Choose a reason for hiding this comment

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

This may work better as an enum for example (just one example where we could improve the api)

@the-mikedavis
Copy link
Member Author

I think we end up using most of the type definitions actually. We could probably drop some of the LSIF stuff like monikers but it wouldn't be a lot of code

@pascalkuthe
Copy link
Member

hmm yeah those two I guess. I was also thinking of dynamic registration stuff. We do need it in some cases where LSP doens't offer any alternative bu t in general I don't really want to implement support for it. I still don't really see the point of it and it's very much optional for a client according to spec.

I think there is also some minor stuff that could be cleanedup even within the code we do use (but don't use some variants). I have some things in mind but it would probably make sense to just switch over to a minimally modified version first. And once that is working we can push cleanups in self contained commits

@the-mikedavis the-mikedavis marked this pull request as ready for review July 28, 2024 14:18
@pascalkuthe pascalkuthe merged commit 3fcf168 into master Jul 31, 2024
6 checks passed
@pascalkuthe pascalkuthe deleted the helix-lsp-types branch July 31, 2024 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants