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

feat: support textDocument/inlayHint request from 3.17.0 spec #566

Merged
merged 4 commits into from Aug 21, 2022

Conversation

rchl
Copy link
Member

@rchl rchl commented Aug 20, 2022

No description provided.

return [];
}

if (!areInlayHintsEnabledForFile(configurationManager, file)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this check necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Otherwise, even with all inlay hint options disabled, the server would still do expensive processing that takes a significant amount of time. The old implementation suffered from that which I've realized when working on this.

@predragnikolic
Copy link
Contributor

predragnikolic commented Aug 20, 2022

Initial testing show that this works ✔️ 👍

Do you see a need to add a check to respect client capabilities if the client supports inlayHints? (IMO I do not see a need for this check, because the client will not send a inlayHints request in the first place if it doesn't support it)

@rchl
Copy link
Member Author

rchl commented Aug 20, 2022

Do you see a need to add a check to respect client capabilities if the client supports inlayHints? (IMO I do not see a need this check, because the client will not send a inlayHints request in the first place if it doesn't support it)

No, I don't see the point. Would only be needed if the server would be the initiating side IMO.

@rchl rchl merged commit 9a2fd4e into master Aug 21, 2022
@rchl rchl deleted the feat/inlay-hints branch August 21, 2022 20:34
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.

None yet

2 participants