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

Implement completionItem/resolve, textDocument/codeAction, textDocument/codeLens and codeLens/resolve #107

Merged
merged 5 commits into from
Feb 25, 2020

Conversation

icsaszar
Copy link
Contributor

Resolves #9

@icsaszar
Copy link
Contributor Author

icsaszar commented Feb 24, 2020

@ebkalderon I'm also working on textDocument/codeAction, textDocument/codeLens and codeLens/resolve. Is it ok if I push them here to merge all of these at once?

Edit: Here's the branch on my repo with the implementation.

src/delegate.rs Outdated Show resolved Hide resolved
Copy link
Owner

@ebkalderon ebkalderon left a comment

Choose a reason for hiding this comment

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

Looks correct to me, I think!

If you'd like to introduce support for the other three LSP request methods in this PR, feel free. I think that because #100 and #58 are now resolved, we no longer need to be overly wary about new methods introducing too much boilerplate. All of these items are somewhat related, so I don't think it's a big deal to group them together.

@ebkalderon
Copy link
Owner

ebkalderon commented Feb 24, 2020

Also, this repository is rebasing merges and not squashing them, meaning we still get to preserve commit-by-commit history. So it's not like having them bundled in one PR ruins the readability of history in master for us. 😄

Ideally, we should have each newly added method be in a separate commit. Feel free to ping me again for a second review, if needed!

Thanks for opening this pull request, @icsaszar.

@icsaszar icsaszar changed the title Implement completionItem/resolve Implement completionItem/resolve, textDocument/codeAction, textDocuemnt/codeLens and codeLens/resolve Feb 24, 2020
@icsaszar
Copy link
Contributor Author

icsaszar commented Feb 24, 2020

@ebkalderon I've added the new methods, everything should be complete now.

This also brings up a small new issue: now that methods are optional by default, it is easy to forget to implement them for Box in lib.rs.

If you're ok with it, I can try to write a macro (I don't have too much experience with macros either) to automatically fill out the impls for Box.

Edit: I just found auto_impl, which seems to be exactly what what we need. The only question is if it works with async_trait.

@ebkalderon
Copy link
Owner

I agree that we should write a macro, but I doubt auto_impl will work with async-trait since the raw syntax generated by the procedural macro will likely not match up with what auto_impl expects. But I believe that should land in a separate PR from this one, since it's not directly related to the task at hand.

Otherwise, thanks a lot for the helpful PR! I think it's ready to merge. 👍

@ebkalderon ebkalderon changed the title Implement completionItem/resolve, textDocument/codeAction, textDocuemnt/codeLens and codeLens/resolve Implement completionItem/resolve, textDocument/codeAction, textDocument/codeLens and codeLens/resolve Feb 25, 2020
@ebkalderon ebkalderon merged commit 8a9f72e into ebkalderon:master Feb 25, 2020
@ebkalderon
Copy link
Owner

Resolves #11 and affects #10.

ebkalderon added a commit that referenced this pull request Feb 25, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
invocation occurs first, before calling `#[async_trait]`, such that the
`#[auto_impl]` macro receives the desugared output of `#[async_trait]`.

#107 (comment)
ebkalderon added a commit that referenced this pull request Feb 25, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
invocation occurs first, before calling `#[async_trait]`, such that the
`#[auto_impl]` macro receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
ebkalderon added a commit that referenced this pull request Feb 25, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
invocation occurs first, before calling `#[async_trait]`, such that the
`#[auto_impl]` macro receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
ebkalderon added a commit that referenced this pull request Feb 25, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
`auto_impl` invocation occurs first, such that the `#[auto_impl]` macro
receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
ebkalderon added a commit that referenced this pull request Feb 27, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
`auto_impl` invocation occurs first, such that the `#[auto_impl]` macro
receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
ebkalderon added a commit that referenced this pull request Feb 28, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
`auto_impl` invocation occurs first, such that the `#[auto_impl]` macro
receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
ebkalderon added a commit that referenced this pull request Feb 28, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
`auto_impl` invocation occurs first, such that the `#[auto_impl]` macro
receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
ebkalderon added a commit that referenced this pull request Aug 3, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
`auto_impl` invocation occurs first, such that the `#[auto_impl]` macro
receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
ebkalderon added a commit that referenced this pull request Aug 3, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
`auto_impl` invocation occurs first, such that the `#[auto_impl]` macro
receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
ebkalderon added a commit that referenced this pull request Aug 3, 2020
It turns out that `auto_impl` can work with `async-trait` as long as the
`auto_impl` invocation occurs first, such that the `#[auto_impl]` macro
receives the desugared output of `#[async_trait]`.

Original suggestion can be found here:
#107 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for completionItem/resolve request on server
2 participants