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.

Sign up for GitHub

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

Jump to bottom

fix: remove usages from vim.lsp.util.parse_snippet #1734

Merged
merged 1 commit into from
Oct 25, 2023
Merged

fix: remove usages from vim.lsp.util.parse_snippet #1734

merged 1 commit into from
Oct 25, 2023

Conversation

MariaSolOs
Copy link
Contributor

This function is now deprecated.

@MariaSolOs
Copy link
Contributor Author

@hrsh7th I would really appreciate a look at this. The warning that these usages generate in nightly neovim are quite disruptive.

@Shougo Shougo mentioned this pull request Oct 23, 2023
Closed
2 tasks
@k1ng440
Copy link

k1ng440 commented Oct 24, 2023

@hrsh7th Please review this. Thank you

@Shougo
Copy link

Shougo commented Oct 25, 2023

He seems busy...

@mehalter
Copy link
Contributor

Yeah it's also only been a couple days. It is open source software and they are maintaining this software for free and for the good of the community. It's probably better for people to not constantly ping them so frequently. If this PR is necessary for your set up, use that fork and branch in the meantime.

@hrsh7th
Copy link
Owner

hrsh7th commented Oct 25, 2023

I'm not sure if "just deleting the existing process" is a reasonable solution, but if we don't merge it, there will likely be a big problem with the ecosystem, so let's merge it.

google translated

@hrsh7th hrsh7th merged commit 51260c0 into hrsh7th:main Oct 25, 2023
1 check passed
@MariaSolOs
Copy link
Contributor Author

@hrsh7th thank you for the review! That being said, I wish to provide some further context around the deprecation.

I'm not sure if "just deleting the existing process" is a reasonable solution, but if we don't merge it, there will likely be a big problem with the ecosystem, so let's merge it.

I was personally against the rapid changes around parse_snippet. However neovim is working really hard on building a better LSP API, and parse_snippet is an ill-designed function. Despite me being very annoyed at my completion configuration suddenly breaking like this, I agree with the rest of the core maintainers that this is the right thing to do.

The issue with this utility function is that it "parses" and formats using undefined heuristics. Its main purpose is to provide the word to use in vim completion items, but there are snippets with no good word replacement (e.g. postfix/multi-line snippets). This is a result of the lack of a 1:1 mapping between vim <-> LSP completion concepts, but from my experience this is a problem all editors that implement the LSP protocol encounter.

Neovim is doing its best to refactor these LSP APIs into robust editor interfaces that are fully compliant with the protocol while still retaining the magical features that neovim allows you to use. IMO we're doing a great job so far, and although this was a band-aid that really hurt ripping, I promise you that it will be worth it.

@MariaSolOs MariaSolOs deleted the parse_snippet branch October 25, 2023 02:15
williamboman added a commit to williamboman/nvim-cmp that referenced this pull request Oct 26, 2023
@williamboman
Merge remote-tracking branch 'upstream/main' into feat/docs-preview-w…
0ece15a 
…indow

* upstream/main:
  fix: remove usages from vim.lsp.util.parse_snippet (hrsh7th#1734)
  ci: fix broken tests (hrsh7th#1729)
  Format with stylua
  docs: Add comments and type annotations for sorting comparators (hrsh7th#1687)
  fix(ghost_text): `ephemeral` causes a false positive of no inline support (hrsh7th#1645)
  perf(core): simplify and improve `find_line_suffix()` (hrsh7th#1675)
  Format with stylua
  feat: add toggle_doc functionality (hrsh7th#1647)
  add context check for invalid detection
  implement is_invalid detection
  perf: fix `nvim_replace_termcodes` being called on every `CursorMoved` (hrsh7th#1650)
  improve pattern handling
  Format with stylua
  fix confirm resolve
@magnuslarsen magnuslarsen mentioned this pull request Dec 5, 2023
Closed
@Shougo Shougo mentioned this pull request Feb 20, 2024
Closed
2 tasks
Hiroya-W pushed a commit to Hiroya-W/nvim-cmp that referenced this pull request May 24, 2024
@MariaSolOs
fix: remove usages from vim.lsp.util.parse_snippet (hrsh7th#1734)
b5b98f2
YoungHaKim7 added a commit to YoungHaKim7/rust_dev_neovide_nvimsetting that referenced this pull request Aug 12, 2024
@YoungHaKim7
https://github.com/hrsh7th/nvim-cmp/pull/1734
84f0f3e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MariaSolOs @k1ng440 @Shougo @mehalter @hrsh7th