-
-
Notifications
You must be signed in to change notification settings - Fork 412
Escape dollar signs in completion snippets #4745
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
base: master
Are you sure you want to change the base?
Conversation
80b26e1 to
d389f5f
Compare
|
It looks like there are some tests to update: |
|
Oh well, I think I'm gonna have to do this properly after all 😃 Completely forgot about record snippets.. |
fa1cf7a to
9f9f289
Compare
696eeef to
2e0e266
Compare
|
Should be good now, I think the test failures are from flaky tests @sgillespie. |
2e0e266 to
af036a0
Compare
|
@Soupstraw Do you want to upstream the If possible, I want to merge this PR before the next HLS release, which will be soon ™️ |
fendor
left a comment
There was a problem hiding this 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.
| data SnippetAny | ||
| = SText Text | ||
| | STabStop Int | ||
| | SPlaceholder Int SnippetAny | ||
| | SChoice Int (NonEmpty Text) | ||
| | SVariable Text (Maybe SnippetAny) | ||
| deriving (Eq, Show) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little more documentation for what the individual constructors mean so that it is easier to follow here.
This PR adds sanitization to completion snippets. It does that by introducing the
Snippettype and asnippetToTextfunction that converts aSnippetto a syntactically valid snippet string.close #4363