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

Implement signatureHelp and implementation #98

Merged
merged 4 commits into from
Feb 21, 2020
Merged

Implement signatureHelp and implementation #98

merged 4 commits into from
Feb 21, 2020

Conversation

icsaszar
Copy link
Contributor

No description provided.

@icsaszar icsaszar mentioned this pull request Feb 20, 2020
Merged
ebkalderon
ebkalderon requested changes Feb 21, 2020
View reviewed changes
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.

Thank you for the helpful pull request, @icsaszar! It all looks great. ❤️

I just have some nitpicks regarding the naming and ordering of the methods and associated types. I try to keep the entries consistent with their definition and declaration in the currently supported LSP specification document as much as possible, so I'd love it if we could rearrange them.

examples/server.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
src/delegate.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@ebkalderon ebkalderon self-assigned this Feb 21, 2020
@icsaszar
Copy link
Contributor Author

Thanks for the feedback, I really appreciate that you care about small details like the order (which I totally agree with since there are a lot of methods and it would be quite hard to navigate without an IDE).

I think i can fix everything today.

@icsaszar
Copy link
Contributor Author

@ebkalderon Everything should be fixed

ebkalderon
ebkalderon approved these changes Feb 21, 2020
View reviewed changes
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.

Terrific work! Thanks for the contribution and for the quick response, @icsaszar. 🚀

@ebkalderon ebkalderon merged commit 6699ac0 into ebkalderon:master Feb 21, 2020
@ebkalderon
Copy link
Owner

Affects #10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebkalderon ebkalderon ebkalderon approved these changes

Assignees

@ebkalderon ebkalderon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@icsaszar @ebkalderon