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): add lsp.util.split_lines and lsp.util.split_lines_iter #16286

Closed
wants to merge 1 commit into from

Conversation

dmitmel
Copy link
Contributor

@dmitmel dmitmel commented Nov 11, 2021

How do I generate documentation for lsp.util.split_lines and lsp.util.split_lines_iter?

---@param bufnr (number)
---@returns (string)
local function buf_get_line_ending(bufnr)
return format_line_ending[nvim_buf_get_option(bufnr, 'fileformat')] or '\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

in what scenario do we need the fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is simply written defensively, AFAIK it is impossible to set fileformat to an invalid value

Copy link
Contributor

@mjlbach mjlbach left a comment

Choose a reason for hiding this comment

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

I need to think more about these. get_buf_text is the only thing I think is 100% necessary or correct.

The issue with \r is internally, we can have \r without a properly tracked newline split. (so we can have midline \r unlike other editors)

runtime/lua/vim/lsp/diagnostic.lua Outdated Show resolved Hide resolved
---
---@see |vim.lsp.util.split_lines_iter()|
---@see https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocuments
function M.split_lines(text, opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we don't internally handle \r according to the specification, I'm not sure this is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you talking about the function or the usages of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm talking about we shouldn't be splitting lines on \r before sending to the server

@dmitmel
Copy link
Contributor Author

dmitmel commented Nov 13, 2021

@mjlbach Alright, I give up. I'll undo the changes to usages of lsp.util.split_lines, but let's at least move the function itself to a different PR. Don't you need something like that in order to handle in-line \rs?

@dmitmel
Copy link
Contributor Author

dmitmel commented Nov 13, 2021

Do any Language Servers return CRLF newlines in textEdits? Asking for whether vim.split(text_edit.newText, '\n', true) should use fileformat-dependent newline too.

@dmitmel
Copy link
Contributor Author

dmitmel commented Nov 13, 2021

but let's at least move the function itself to a different PR. Don't you need something like that in order to handle in-line \rs?

Nevermind, you can just nvim_buf_get_lines and split each line on \r. The only place where I imagine this function could be useful is for lsp.util.get_lines (and a similar one in lsp.diagnostic which can be refactored to use lsp.util.get_lines), which can't do fileformat detection. Please tell me if you need split_lines - if not, I am pulling it out.

@dmitmel dmitmel changed the title fix(lsp): refactor text line splitting fix(lsp): join buffer contents on fileformat-dependent line break character instead of \n Nov 13, 2021
@dmitmel dmitmel changed the title fix(lsp): join buffer contents on fileformat-dependent line break character instead of \n feat(lsp): add lsp.util.split_lines and lsp.util.split_lines_iter Nov 13, 2021
@dmitmel dmitmel changed the title feat(lsp): add lsp.util.split_lines and lsp.util.split_lines_iter feat(lsp): add lsp.util.split_lines and lsp.util.split_lines_iter Nov 13, 2021
@dmitmel
Copy link
Contributor Author

dmitmel commented Nov 13, 2021

@mjlbach What do we do with this PR? (Ref: #16277 (comment))

@mjlbach
Copy link
Contributor

mjlbach commented Nov 13, 2021

WDYM what do we do? I need to think about some of the thing's bjorn said about our endline handling, a lot of my internal justification for maintaining the line ending when communicating with the language server is undermined by the fact we save the buffer to disk as utf-8

@dmitmel
Copy link
Contributor Author

dmitmel commented Nov 13, 2021

Well, I mean, should I make it a PR with just a fix for #16231 and move split_lines to a different PR, or keep both in this PR, or something else? I have undone the changes to usage of split_lines because I couldn't find a server which reports back CR or CRLF line endings on hover, signature help, diagnostics or something of that matter, i.e. unrelated to file content tracking.

@dmitmel
Copy link
Contributor Author

dmitmel commented Nov 20, 2021

Moved the buf_get_full_text fix to #16334

@mfussenegger
Copy link
Member

Closing this because it conflicts with #25272 and the functions added aren't used by other lsp components

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

4 participants