Skip to content
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

add double-quote escape in $t/$j comments #91

Merged
merged 3 commits into from
Jul 3, 2023
Merged

add double-quote escape in $t/$j comments #91

merged 3 commits into from
Jul 3, 2023

Conversation

digama0
Copy link
Member

@digama0 digama0 commented Jun 9, 2022

This adds support for strings like "ab""c" in $t and $j comments which encode the string ab"c.

Some additional care is required in user code, since now the body of a string is no longer the actual contents of the string but rather doubled characters have to be unescaped. We return a Cow now and check for the very common case of no escapes to avoid the copy most of the time. But if working directly with string spans, CommandToken::unescape_string should be used to get the value now.

@tirix
Copy link
Collaborator

tirix commented Feb 7, 2023

(Coming back to this after a long break!)
The logic of this PR is good, even if I don't see a current use-case.

My only issue would be with the changes in grammar.rs, which seem to be unrelated, and conflicting with other changes in main.
Could you revert the changes in grammar.rs / move them to a different PR?

@jkingdon
Copy link
Contributor

It would appear this correctly handles the following cases from the Metamath Book (section 4.4.2), which has a fair amount of detail about what the rules are supposed to be:

Example Meaning
"a""b" a"b
’c’’d’ c’d
"e’’f" e’’f
’g""h’ g""h

@tirix
Copy link
Collaborator

tirix commented Jul 2, 2023

@jkingdon @digama0 shouldn't we merge this PR? What is blocking here?

@jkingdon
Copy link
Contributor

jkingdon commented Jul 2, 2023

@jkingdon @digama0 shouldn't we merge this PR? What is blocking here?

Not sure I looked carefully enough (either by code inspection, or by writing unit tests) to provide too much input, but I don't object.

@digama0 digama0 merged commit da8cc7e into main Jul 3, 2023
@digama0 digama0 deleted the double_quote branch July 3, 2023 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants