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(lsp): allow enable/disable inlay_hint by client_id #28521

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ofseed
Copy link
Contributor

@ofseed ofseed commented Apr 26, 2024

The original idea was to provide API consistency, and if this was implemented then users could use

if client.name == "lua_ls" then
  vim.lsp.inlay_hint.enable(not vim.lsp.inlay_hint.is_enabled(), { client_id = client.id })
end

to enable inlay hints only for desired clients. Though you can close it on the server side, this provides more flexibility.

@@ -1649,13 +1651,14 @@ get({filter}) *vim.lsp.inlay_hint.get()*
• {client_id} (`integer`)
• {inlay_hint} (`lsp.InlayHint`)

is_enabled({bufnr}) *vim.lsp.inlay_hint.is_enabled()*
is_enabled({bufnr}, {client_id}) *vim.lsp.inlay_hint.is_enabled()*
Copy link
Member

Choose a reason for hiding this comment

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

if more filters are being added then it's time to make this a "kwargs" arg instead of adding more parameters.

probably is_enabled({filter}) is best, though is_enabled({bufnr}, {filter}) could be acceptable (and bufnr=nil would be "all/any buffer")

Suggested change
is_enabled({bufnr}, {client_id}) *vim.lsp.inlay_hint.is_enabled()*
is_enabled({bufnr}, {filter}) *vim.lsp.inlay_hint.is_enabled()*

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

If I understand the changes right, the same could currently be accomplished by changing the capabilities in the vim.lsp.start calls so that only the clients you want to use for inlay-hints indicate support for it.

Wouldn't that be good enough?

On a glance the diff looks quite impactful, I'd rather not merge this close to 0.10, especially not without tests covering this(?)

@ofseed
Copy link
Contributor Author

ofseed commented Apr 26, 2024

Honestly, I originally wanted to create an issue to ask if it was worth completing, but I didn't want to use an issue to describe this feature, so I implemented it directly. Now it seems that the current implementation is indeed rather sloppy.

On a glance the diff looks quite impactful, I'd rather not merge this close to 0.10,

I know it's better to improve existing features rather than risk adding new ones now, but if we change is_enabled({bufnr}, {filter}) to is_enabled({filter}) after the release of 0.10, the breaking change will impact the stable users.

By the way, the breaking change I originally thought was considering removing vim.lsp.inlay_hint.get.ret

--- Optional filters |kwargs|:
--- @class vim.lsp.inlay_hint.get.Filter
--- @inlinedoc
--- @field bufnr integer?
--- @field range lsp.Range?
--- @class vim.lsp.inlay_hint.get.ret
--- @inlinedoc
--- @field bufnr integer
--- @field client_id integer
--- @field inlay_hint lsp.InlayHint

because it would be redundant if this PR is merged. (users know what bufnr and cilent_id is when they call this function)

especially not without tests covering this(?)

If this feature is acceptable, I'm willing to add a lot of tests in this PR.

@justinmk
Copy link
Member

but if we change is_enabled({bufnr}, {filter}) to is_enabled({filter}) after the release of 0.10, the breaking change will impact the stable users.

we can do that in a backwards-compatible way without much trouble.

@ofseed
Copy link
Contributor Author

ofseed commented Apr 26, 2024

I created #28523, convert this into a draft PR now.

@ofseed
Copy link
Contributor Author

ofseed commented May 4, 2024

If I understand the changes right, the same could currently be accomplished by changing the capabilities in the vim.lsp.start calls so that only the clients you want to use for inlay-hints indicate support for it.

Wouldn't that be good enough?

We cannot toggle inlay hints by clients in this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants