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

Fix rust escape double quots in string #3904

Conversation

MohamedAli00949
Copy link
Contributor

@MohamedAli00949 MohamedAli00949 commented Oct 29, 2023

I have fixed unescaping the " at string attributes such as I have seen in the #3817 issue.

Changes

The string scops didn't contain the backslash escape expression which is BACKSLASH_ESCAPE so I have put it at every string scope.

Before
image

After
rust-issue

Checklist

  • Add BACKSLASH_ESCAPE to string scops to avoid this issue.
  • Add markup tests to test the validation of the solution.

@github-actions
Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

4 files changed

Total change +45 B

View Changes
file base pr diff
es/core.min.js 8.13 KB 8.14 KB +1 B
es/highlight.min.js 8.13 KB 8.14 KB +1 B
es/languages/rust.min.js 1.43 KB 1.45 KB +21 B
languages/rust.min.js 1.44 KB 1.46 KB +22 B

@MohamedAli00949
Copy link
Contributor Author

MohamedAli00949 commented Oct 30, 2023

hi @joshgoebel, this is my suggested solution to solve the resolved issue feel free to review it and give me your pieces of advice.

Comment on lines 196 to 208
{
begin: /b?r(#*)"(.|\n)*?"\1(?!#)/,
contains: [
hljs.BACKSLASH_ESCAPE
]
},
{
begin: /b?'\\?(x\w{2}|u\w{4}|U\w{8}|.)'/,
contains: [
hljs.BACKSLASH_ESCAPE
]
},
],
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work... you'd need end as well and need to rewrite any specific internals (like the unicode matching) to be sub rules inside the contains. And you can't use match history (\1) across begin/end combos... how many # characters are used in practice? is there a max limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry, I can't understand you.
Do you mean removing this change avoiding complexity or another thing?

Copy link
Member

Choose a reason for hiding this comment

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

Please add test cases for these and you will see that they do not work they way they are currently written.

Copy link
Contributor Author

@MohamedAli00949 MohamedAli00949 Nov 4, 2023

Choose a reason for hiding this comment

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

You are right, this change didn't work. So I will remove contains at these variants and commit changes again.
Thanks in advance

Copy link

github-actions bot commented Nov 4, 2023

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change +36 B

View Changes
file base pr diff
es/core.min.js 8.14 KB 8.14 KB -1 B
es/highlight.min.js 8.14 KB 8.14 KB -1 B
es/languages/rust.min.js 1.43 KB 1.45 KB +19 B
highlight.min.js 8.17 KB 8.17 KB -1 B
languages/rust.min.js 1.44 KB 1.46 KB +20 B

@joshgoebel
Copy link
Member

Ok, I supposing fixing one of the variants s better than nothing. Please add a changelog entry.

@MohamedAli00949
Copy link
Contributor Author

@joshgoebel, I have updated the changelogs at CHANGES.md Is that what you expect?

@joshgoebel
Copy link
Member

Please only include your single change in the log.

@MohamedAli00949
Copy link
Contributor Author

@joshgoebel, Thanks for approving the changes, I hope to merge it if possible.

@MohamedAli00949
Copy link
Contributor Author

@joshgoebel, I show that the first test has failed and I can't find the reason for that I want to understand that if you can please explain that with my thanks in advance.

@joshgoebel joshgoebel merged commit 3270ef9 into highlightjs:main Nov 7, 2023
13 of 14 checks passed
Copy link

github-actions bot commented Nov 7, 2023

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

3 files changed

Total change +38 B

View Changes
file base pr diff
es/languages/rust.min.js 1.43 KB 1.45 KB +19 B
highlight.min.js 8.17 KB 8.17 KB -1 B
languages/rust.min.js 1.44 KB 1.46 KB +20 B

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.

2 participants