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.

Sign up for GitHub

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

Jump to bottom

refactor(cli/lsp): migrate from lspower back to tower-lsp #14059

Closed
wants to merge 39 commits into from
Closed

refactor(cli/lsp): migrate from lspower back to tower-lsp #14059

wants to merge 39 commits into from

Conversation

CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Mar 21, 2022 •
edited
Loading

closed: #14056

@CGQAQ CGQAQ marked this pull request as draft March 21, 2022 07:28
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Mar 21, 2022

@kitsonk tower_lsp seems don't have this method, what should I do?

CGQAQ
CGQAQ commented Mar 21, 2022
View reviewed changes
Copy link
Contributor Author

@CGQAQ CGQAQ left a comment •
edited
Loading

Choose a reason for hiding this comment

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

is this ok?. @kitsonk

@CGQAQ CGQAQ marked this pull request as ready for review March 21, 2022 10:22
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Mar 21, 2022

I think it’s ready for review

kitsonk
kitsonk suggested changes Mar 21, 2022
View reviewed changes
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

Thank you for the effort.

Some feedback. Mostly, let's just import lsp_types as lsp to avoid a huge amount of churn in the code, plus it reads a lot better that way.

cli/bench/lsp.rs Outdated Show resolved Hide resolved
cli/lsp/analysis.rs Outdated Show resolved Hide resolved
cli/lsp/client.rs Outdated Show resolved Hide resolved
cli/lsp/code_lens.rs Outdated Show resolved Hide resolved
cli/lsp/completions.rs Outdated Show resolved Hide resolved
cli/lsp/tsc.rs Outdated Show resolved Hide resolved
cli/lsp/tsc.rs Outdated Show resolved Hide resolved
cli/tests/integration/lsp_tests.rs Outdated Show resolved Hide resolved
cli/tests/integration/lsp_tests.rs Outdated Show resolved Hide resolved
cli/tests/integration/lsp_tests.rs Outdated Show resolved Hide resolved
CGQAQ and others added 14 commits March 21, 2022 19:24
@CGQAQ @kitsonk
Update cli/bench/lsp.rs
99965d7 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/analysis.rs
279c7ff 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/client.rs
f4eb740 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/code_lens.rs
a3d1f4d 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/completions.rs
779b1e5 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/tests/integration/lsp_tests.rs
fe8a5ae 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/tests/integration/lsp_tests.rs
22e221a 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/config.rs
df92968 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/documents.rs
ff51a93 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/lsp_custom.rs
25b30a7 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/refactor.rs
556584b 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/registries.rs
023f911 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/text.rs
8e4d9c9 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ @kitsonk
Update cli/lsp/tsc.rs
bba7786 
Co-authored-by: Kitson Kelly <me@kitsonkelly.com>
@CGQAQ CGQAQ requested a review from kitsonk March 21, 2022 12:32
bartlomieju
bartlomieju reviewed Mar 21, 2022
View reviewed changes
Cargo.lock Outdated Show resolved Hide resolved
kitsonk
kitsonk suggested changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

We can't break the API this way. We need tower_lsp to support other requests, versus trying to repurpose an existing lsp method.

cli/lsp/language_server.rs Outdated Show resolved Hide resolved
@kitsonk
Copy link
Contributor

kitsonk commented Mar 29, 2022

tower_lsp added the feature for custom server requests here: ebkalderon/tower-lsp#313. We need to implement it that way.

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Mar 29, 2022 •
edited
Loading

Ok, I'll change the tower-lsp from 0.15.1 to 0.16

image

it seems not possible to made the upgrade

image

@CGQAQ CGQAQ marked this pull request as draft March 30, 2022 09:12
@CGQAQ CGQAQ mentioned this pull request Mar 31, 2022
Merged
@CGQAQ CGQAQ closed this Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju left review comments

@kitsonk kitsonk kitsonk requested changes

@dsherret dsherret Awaiting requested review from dsherret

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lsp: migrate from lspower back to tower-lsp
3 participants
@CGQAQ @kitsonk @bartlomieju