Skip to content

Conversation

@Soupstraw
Copy link
Contributor

@Soupstraw Soupstraw commented Oct 23, 2025

This PR adds sanitization to completion snippets. It does that by introducing the Snippet type and a snippetToText function that converts a Snippet to a syntactically valid snippet string.

close #4363

@sgillespie
Copy link
Collaborator

It looks like there are some tests to update:

ghcide
  completion
    topLevel
      recordsConstructor: FAIL (1.06s)
        ghcide-test/exe/CompletionTests.hs:112:
        expected: [("XyRecord",Just CompletionItemKind_Constructor,Just "XyRecord",Nothing),("XyRecord",Just CompletionItemKind_Snippet,Just "XyRecord {x=${1:_x}, y=${2:_y}}",Nothing)]
         but got: [("XyRecord",Just CompletionItemKind_Constructor,Just "XyRecord",Nothing),("XyRecord",Just CompletionItemKind_Snippet,Just "XyRecord {x=\\${1:_x}, y=\\${2:_y}}",Nothing)]

@Soupstraw
Copy link
Contributor Author

Soupstraw commented Oct 23, 2025

Oh well, I think I'm gonna have to do this properly after all 😃

Completely forgot about record snippets..

@Soupstraw Soupstraw marked this pull request as draft October 23, 2025 16:49
@Soupstraw Soupstraw marked this pull request as ready for review October 23, 2025 17:25
@Soupstraw Soupstraw force-pushed the sanitize-snippets branch 3 times, most recently from 696eeef to 2e0e266 Compare October 23, 2025 18:44
@Soupstraw
Copy link
Contributor Author

Should be good now, I think the test failures are from flaky tests @sgillespie.

@fendor
Copy link
Collaborator

fendor commented Nov 24, 2025

@Soupstraw Do you want to upstream the StructuredCompletionItem to lsp or do you want to rather merge it into HLS as is and think about it later in follow-up work?

If possible, I want to merge this PR before the next HLS release, which will be soon ™️

@fendor fendor self-assigned this Nov 24, 2025
@fendor fendor added the status: needs review This PR is ready for review label Nov 24, 2025
@fendor fendor requested review from fendor and sgillespie November 24, 2025 17:00
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, I am happy to merge once SnippetAny has some docs.

@Soupstraw
Copy link
Contributor Author

Soupstraw commented Dec 1, 2025

@Soupstraw Do you want to upstream the StructuredCompletionItem to lsp or do you want to rather merge it into HLS as is and think about it later in follow-up work?

If possible, I want to merge this PR before the next HLS release, which will be soon ™️

@fendor I made an issue in the lsp repo about upstreaming this work (haskell/lsp#626 (comment)). I think I'd rather have this PR merged now and then doing a follow-up once it's available upstream.

@fendor fendor merged commit 3a2b23e into haskell:master Dec 1, 2025
34 of 36 checks passed
@fendor
Copy link
Collaborator

fendor commented Dec 1, 2025

Perfect, thank you very much! If you find the time, could you open a ticket in HLS as well that we want rather the lsp stuff merged and use that?

@Soupstraw
Copy link
Contributor Author

Perfect, thank you very much! If you find the time, could you open a ticket in HLS as well that we want rather the lsp stuff merged and use that?

Done: #4772

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs review This PR is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid snippet syntax causes failures in neovim text editor

3 participants