Support for escaped quotes #23
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmfxyz
commented
May 22, 2022
jams.js
Outdated
| if (child.type == "unsafe") { | ||
| // EBNF module re-escapes strings, so "\" becomes "\\" | ||
| // iff child is of unsafe type do we re-do the unescape | ||
| let unescaped = child.text.replace(/\\/, "") |
There was a problem hiding this comment.
I know we reverted this before, but now we only do it for unsafe children.
|
This is a good find. Thank you, please file an issue in their repo and point here |
|
@nmushegian see here for more issue and example: lys-lang/node-ebnf#41 |
nmushegian
reviewed
May 22, 2022
| o = jams(`{key "val"}`) | ||
| t.equal(o.key, "val") | ||
| o = jams(String.raw`{"\"key" "\"val"}`) // Need raw so js doesn't escape \ | ||
| t.equal(o[`"key`], `"val`) |
There was a problem hiding this comment.
This test shouldn’t even pass based on latest requirement
The point is to separate out bareword part and at least get that done first
Also global replace like this is probably never the right solution
|
This is too many changes in one PR |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR includes changes from #22, that one can be merged first to see true diff.
There is a problem with the ebnf module we use -- it auto escapes strings somewhere in its implementation. For example the string:
is re-escaped to
within the ebnf module itself.
So to address this I implemented
unsafetype (only supports escaped quotes for now)unsafeto exist withinquotequotechild by child, and replacing the re-escaped characters only if the child is of typeunsafeWe could redefine the grammar to expect the double
\\, but this would be an inaccurate grammar tailored to our specific parser.Added new file based tests that are all passing. If we have a list of other characters that should be escaped, I can add them too.