Handle special characters inside \text commands. #7
Conversation
e.g. \text{(english text)} would fail because
the unescaped paretheses were messing up the regexes
| @@ -374,6 +374,16 @@ function rtrim(str) { | |||
| return str.replace(/\s+$/g, ''); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Lets not extend the RegExp object, but instead make this into a separate function, perhaps escapeForRegex?
Can you also please add a sentence at the top of this jsdoc comment to briefly describe what this function does, for example something like:
/**
* Escape any string to create a regular expression.
*
* See: https://stackoverflow...
| @@ -996,6 +996,39 @@ describe('TranslationAssistant (\\text{}, \\textbf{})', function() { | |||
| translated: '', | |||
| }], ['$\\textbf{Fl盲che}} = 12 \\textbf { Quadratzentimeter}$']); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
Thank you for creating this fix @danielhollas
I left a request for change inline below, but otherwise it looks great.
Can you also please update the commit summary with a possible translations editor link where this fix can be tested live (if possible)?
|
Awesome, will do. 馃憤 Sorry for a newbie question, but what is the process now? Should I make the requested changes and push a new commit (but where should I put the new summary?), or should I squash the commits and force-push to this branch? |
|
You can make the code changes in a new commit, with the "final" summary (that can include the test plan link to verify fix in translations editor). When I accept the pull request, I'll choose "Squash and merge" and pick the commit summary from your last commit (with the requested changes), this should end up in |
Summary:
The code was failing when NL text inside $\text{}$
contained special regex character like () or .
https://khanacademy.atlassian.net/browse/IC-129
Test plan: - Go to
`https://www.khanacademy.org/translations/edit/cs//e/negative-number-addition-and-subtraction-equivalent-expressions/by-pattern`
ST should work on group of strings like this one:
`https://crowdin.com/translate/khanacademy/26580/enus-cs#5294463`
|
Okay, pushed the changes. Actually, you probably won't need to rebuild after all. Per this doc, there should be some parameter after the '--loose' option. So this works for me: I did not commit this change but I do not understand why it works for you without that parameter. |
@danielhollas, you're right! It's no longer working for me either without I don't see a difference in output for |
|
Yep, I pushed the change to #8. Actually, only now I realized how |
ST for
\text{(english text)}would fail because the unescaped parentheses were messing up the regexes.The issue was reported on JIRA IC-129.
(Note of caution, this is my very first pull request so hopefully I am not messing something up 馃檪)
PS: I think I signed the CLA couple months ago...