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

The culling of vim.lsp.util #25272

Open
gpanders opened this issue Sep 20, 2023 · 6 comments
Open

The culling of vim.lsp.util #25272

gpanders opened this issue Sep 20, 2023 · 6 comments
Labels
lsp refactor changes that are not features or bugfixes
Milestone

Comments

@gpanders
Copy link
Member

gpanders commented Sep 20, 2023

Problem

vim.lsp.util is a dumpster that needs to be cleaned up. This process has already been started in #25073. But the pruning must continue.

This is a tracking issue for substantially reducing the size and scope of vim.lsp.util. Ideally, it will be removed completely (deprecated first, of course, and removed in a future release).

In particular, new homes should be found for:

  • _normalize_markdown
    • This should be made public, but not under vim.lsp.util. TBD on the correct namespace for this
  • open_floating_preview
  • make_floating_popup_options
    • This should just be part of vim.ui.open_float. Deprecate.
  • _make_floating_popup_size
    • Same as above.
  • _str_utfindex_enc and _str_byteindex_enc
    • These are advertised as "convenience wrappers" around vim.str_utfindex and vim.str_byteindex, respectively. Consider updating those functions to accept an encoding instead.
  • set_lines
    • I don't see any usages of this in our own code. This should be deprecated and deleted. Plugins can do this themselves, it is not that hard, and we don't need to provide little utility functions for every possible thing that can be done.
  • apply_text_edits
  • _get_completion_item_kind_name
    • This is a one line function. Why is this needed? Can it be inlined/deleted? @mfussenegger
  • text_document_completion_list_to_complete_items
    • The name alone suggests this is a special purpose function and doesn't belong in "util". Is there a more appropriate location?
  • stylize_markdown
    • Deprecate once normalize_markdown is public and vim.ui.open_float exists. At that point this is no longer needed or necessary.

The list above is non-exhaustive, it may be updated with new items as the effort progresses.

@gpanders gpanders added enhancement feature request refactor changes that are not features or bugfixes lsp and removed enhancement feature request labels Sep 20, 2023
@MariaSolOs
Copy link
Member

try_trim_markdown_code_blocks also feels like an utility function that we should get rid of.

@mfussenegger
Copy link
Member

_get_completion_item_kind_name
This is a one line function. Why is this needed? Can it be inlined/deleted? @mfussenegger

Looks like it was due to testing: 9a67b03
But inlining sounds good to me - or a candidate for ⬇️

text_document_completion_list_to_complete_items
The name alone suggests this is a special purpose function and doesn't belong in "util". Is there a more appropriate location?

There is some discussion around introducing a lsp.completion module. My preference would be to then make it private within that module. See #24661 (comment)

_str_utfindex_enc and _str_byteindex_enc
These are advertised as "convenience wrappers" around vim.str_utfindex and vim.str_byteindex, respectively. Consider > updating those functions to accept an encoding instead.

Another potential alternative would be to mix this into some kind of position or range abstraction. I have some vague idea about that, but won't be able to try it anytime soon.

+1 for all the others.

@justinmk
Copy link
Member

justinmk commented Sep 21, 2023

_normalize_markdown should be made public, but not under vim.lsp.util. TBD on the correct namespace

  • We need a vim.text module for various text manipulation things (like vim.text.indent(), vim.text.dedent()).
    • Counterpoint: normalize_markdown is markdown specific so it should live in vim.filetype.markdown ?
      • But markdown is common enough that I'd be ok with cramming it into vim.text. The door is still open for filetype-specific utils in vim.filetype.xx in the future.

Another potential alternative would be to mix this into some kind of position or range abstraction.

Reference: #25509

💯 for the other suggestions in this issue, with these comments:

  • Should the open_float features be owned by nvim_open_win instead of adding vim.ui.open_float ?
  • Are both vim.ui.open_win + vim.ui.open_float needed, or could vim.ui.open_win ergonimically cover both (float and non-float) use-cases?

@justinmk justinmk added this to the backlog milestone Sep 21, 2023
@clason
Copy link
Member

clason commented Sep 21, 2023

Counterpoint: normalize_markdown is markdown specific

But it renders markdown as text, so vim.text would arguably fit.

(I'm wary of opening the floodgates for shoving functionality into vim.filetype.<ft> sub-submodules.

👍 for Taking vim.region Seriously.

@gpanders
Copy link
Member Author

Should the open_float features be owned by nvim_open_win instead of adding vim.ui.open_float ?

I've no objection, though having more of this done in Lua rather than in C might be nice.

Are both vim.ui.open_win + vim.ui.open_float needed, or could vim.ui.open_win ergonimically cover both (float and non-float) use-cases?

That seems reasonable to me.

@gpanders
Copy link
Member Author

gpanders commented Oct 5, 2023

I created #25514 to track the vim.ui.open_win() (and related) design, specifically.

mfussenegger added a commit to mfussenegger/neovim that referenced this issue Oct 21, 2023
To reduce cross-chatter between modules and for neovim#25272
Also preparing for neovim#25714
mfussenegger added a commit to mfussenegger/neovim that referenced this issue Oct 21, 2023
To reduce cross-chatter between modules and for neovim#25272
Also preparing for neovim#25714
mfussenegger added a commit to mfussenegger/neovim that referenced this issue Oct 21, 2023
mfussenegger added a commit that referenced this issue Oct 21, 2023
To reduce cross-chatter between modules and for #25272
Also preparing for #25714
mfussenegger added a commit that referenced this issue Oct 21, 2023
jaehoonhwang pushed a commit to jaehoonhwang/neovim that referenced this issue Oct 22, 2023
To reduce cross-chatter between modules and for neovim#25272
Also preparing for neovim#25714
jaehoonhwang pushed a commit to jaehoonhwang/neovim that referenced this issue Oct 22, 2023
jaehoonhwang pushed a commit to jaehoonhwang/neovim that referenced this issue Oct 22, 2023
To reduce cross-chatter between modules and for neovim#25272
Also preparing for neovim#25714
jaehoonhwang pushed a commit to jaehoonhwang/neovim that referenced this issue Oct 22, 2023
justinmk added a commit to justinmk/neovim that referenced this issue Nov 25, 2023
Problem:
`vim.lsp.buf.definition()` throws an error if prompt is shown while
opening a file (specifically the "E325: ATTENTION" swapfile prompt).

    Error executing vim.schedule lua callback: …/runtime/lua/vim/lsp/util.lua:1074:
    Cursor position outside buffer
    stack traceback:
        [C]: in function 'nvim_win_set_cursor'
        …/runtime/lua/vim/lsp/util.lua:1074: in function 'jump_to_location'
        …/runtime/lua/vim/lsp/handlers.lua:423: in function 'handler'
        …/runtime/lua/vim/lsp.lua:1517: in function ''
        vim/_editor.lua: in function <vim/_editor.lua:0>

Solution:
Handle `nvim_win_set_cursor` failure.

See also: neovim#25272
justinmk added a commit to justinmk/neovim that referenced this issue Nov 25, 2023
Problem:
`vim.lsp.buf.definition()` throws an error if prompt is shown while
opening a file (specifically the "E325: ATTENTION" swapfile prompt).

    Error executing vim.schedule lua callback: …/runtime/lua/vim/lsp/util.lua:1074:
    Cursor position outside buffer
    stack traceback:
        [C]: in function 'nvim_win_set_cursor'
        …/runtime/lua/vim/lsp/util.lua:1074: in function 'jump_to_location'
        …/runtime/lua/vim/lsp/handlers.lua:423: in function 'handler'
        …/runtime/lua/vim/lsp.lua:1517: in function ''
        vim/_editor.lua: in function <vim/_editor.lua:0>

Solution:
Handle `nvim_win_set_cursor` failure.

See also: neovim#25272
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp refactor changes that are not features or bugfixes
Projects
None yet
Development

No branches or pull requests

5 participants