Skip to content

fix: move additionalTextEdits switch #397

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: move additionalTextEdits switch #397

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e463c47
fix: move additionalTextEdits switch
Shane-XB-Qian Sep 19, 2023
a07a550
none: check 'completionTextEdit' when non 'additionalTextEdits' as well
Shane-XB-Qian Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion autoload/lsp/completion.vim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,17 +582,18 @@ def LspCompleteDone(bnr: number)

var completionData: any = v:completed_item->get('user_data', '')
if completionData->type() != v:t_dict
|| !opt.lspOptions.completionTextEdit
return
endif

if !completionData->has_key('additionalTextEdits')
\ && opt.lspOptions.completionTextEdit
# Some language servers (e.g. typescript) delay the computation of the
# additional text edits. So try to resolve the completion item now to get
# the text edits.
completionData = lspserver.resolveCompletion(completionData, true)
endif
if !completionData->get('additionalTextEdits', {})->empty()
\ && opt.lspOptions.completionTextEdit
Copy link
Contributor

@vimpostor vimpostor Dec 19, 2023

Choose a reason for hiding this comment

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

Moving the line here seems a bit fishy to me for another reason:

Suppose that completionTextEdit == false, then only guarding this if condition may perform redundant work. The delayed lspserver.resolveCompletion directly above this if-block might still be executed, but this is completely unnecessary as we won't use the result anywhere.

Copy link
Contributor Author

@Shane-XB-Qian Shane-XB-Qian Dec 19, 2023

Choose a reason for hiding this comment

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

well, simplest change i had said in #389 (comment) (but ignore that cursor issue, you had refined it), if just added and moved 'command' part before 'textedit', then this PR no required and nothing needed change;
i am still not sure what case required such 'command' action at here, could you verify your faced case to verify if it's ok to move that part to there (of course please be sure 'command' if was really required)?

Copy link
Contributor

@vimpostor vimpostor Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if just added and moved 'command' part before 'textedit', then nothing needed change

I already told you in my original PR that this is not possible, the command has to be applied after the textedit.

if it's ok to move that part to there

With the latest commit a07a550 your PR seems fine to me, although I am not an expert with snippet plugins, as I don't use any.

Copy link
Contributor Author

@Shane-XB-Qian Shane-XB-Qian Dec 20, 2023

Choose a reason for hiding this comment

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

if just added and moved 'command' part before 'textedit', then nothing needed change

I already told you in my original PR that this is not possible, the command has to be applied after the textedit.

yes, but i hear I am not entirely sure in that comment.

or now anyway seems only 'haskell' (as you said) worked like this, i have no idea any others, and seems you confirmed it should work like this way, then i think this PR is ready to go.

textedit.ApplyTextEdits(bnr, completionData.additionalTextEdits)
endif

Expand Down