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

chore: Upgrade from deprecated buf_request to buf_request_all for LSP #115

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

johmsalas
Copy link
Owner

closes #57

Copy link
Collaborator

@peterfication peterfication left a comment

Choose a reason for hiding this comment

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

Nice :)

total_files = vim.tbl_count(response.result.changes)
end
end
print(string.format("Changed %s file%s. To save them run ':wa'", total_files, total_files > 1 and "s" or ""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can only print something if it changed more than one file? For one file, I wouldn't care if it's saved or not as I'm working on that specific file anyways right now.

Also, a prefix might be helpful e.g. "Textcase: Changed ..."

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I think about it again, even a message for a single would be interesting because it gives feedback that the LSP rename worked.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's the reason for this comment.

There is currently a bug when multiple clients do textDocument/rename, the solution for this will be, given the nature of textDocument/rename it should only apply one of the clients, not all of them, I'll add some logic to chose one of the clients in the next PR

I didn't include it in this PR to separate the deliveries into small chunks, but in fact, I wouldn't have prioritized buf_request_all into the first milestone if it wasn't part of the solution to the multi LSP clients issue

Copy link
Owner Author

Choose a reason for hiding this comment

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

@peterfication by the way, I removed my assignment for #107 because I know you like to learn and might be interested on the final piece of this fix. Let me know otherwise or assign it to me directly :)

I've considered as a tech approach to iterate the list of results, find the one having the biggest amount of changes. In theory, all of them should provide exactly the same amount of changes, but just to be safe I'd still find the client that found the biggest amount. Then apply textDocument/rename using that client

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, thanks for the explanation :)

I would really like to dive into #107

-- after the edits are applied, the files are not saved automatically.
-- let's remind ourselves to save those...
-- TODO: This will be modified to include only one of the clients count
total_files = vim.tbl_count(response.result.changes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't fully understand this line. Wouldn't it make sense to add up all the counts?

@johmsalas johmsalas merged commit 6210060 into main Dec 15, 2023
6 checks passed
@johmsalas johmsalas deleted the issue-57 branch December 15, 2023 12:23
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.

vim.lsp.buf_request is deprecated
2 participants