-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Robust snippet parsing #6371
Robust snippet parsing #6371
Conversation
2fe53dc
to
4dab531
Compare
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.
Looks good to me.
fn anything<'a>(escape_chars: &'static [char]) -> impl Parser<'a, Output = SnippetElement<'a>> { | ||
fn anything<'a>( | ||
escape_chars: &'static [char], | ||
end_at_brace: bool, |
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.
bool
as parameter aren't pretty. Maybe directly specifying a const instead?
end_at_brace: bool, | |
extra_chars: &'static [char], |
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.
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.
Ah sorry, I meant to answer this but somehow my comment didn't get posted. Even with const generics this is not that easy to do since we need to add $
to the slice. It's possible to do this with a bunch of fancy array tricks + const generic magic (or by allocating a vec) but I think a simple bool
parameter is preferable to that. If we actually need something else than a bracket here we can revisit
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.
Thanks for expanding the test cases, this looks great!
Closes #6347
Fixes a bunch of small bugs in the snippet parsing, mostly relating to escape sequences. The LSP standard is incomplete, ambigous/missleading and sometimes even downright wrong so I just used the vscode implementation as a reference. I have started porting a bunch of tests over from there to be sure we are doing the right thing now. However due to the sheer number of tests I stopped with that at some point to preserver my sanity.
The large number of inserted lines are just tests, the code changes itself are pretty minor.