-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
i18n: make double dollar validation less strict #10299
i18n: make double dollar validation less strict #10299
Conversation
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 like a good start. I think there might also need to be some tests to make sure this catches double dollars and allows adjacent ICU strings. e.g.
example of a $$ bad string
example of a $ICU_0$$ICU_1$ good string
example of another $$ICU_0$ good string
example of a $ICU_0$$ICU_1$ $$ mixed string
@@ -342,7 +342,8 @@ function _processPlaceholderDirectIcu(icu, examples) { | |||
*/ | |||
function _ctcSanityChecks(icu) { | |||
// '$$' i.e. "Double Dollar" is always invalid in ctc. | |||
if (icu.message.match(/\$\$/)) { | |||
const regexMatch = icu.message.match(/\$([^$]*?)\$/); |
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.
This should be a global match to check for multiple double dollars.
@@ -342,7 +342,8 @@ function _processPlaceholderDirectIcu(icu, examples) { | |||
*/ | |||
function _ctcSanityChecks(icu) { | |||
// '$$' i.e. "Double Dollar" is always invalid in ctc. | |||
if (icu.message.match(/\$\$/)) { | |||
const regexMatch = icu.message.match(/\$([^$]*?)\$/); | |||
if (regexMatch && !regexMatch[1]) { |
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.
Will need to iterate over all matches.
@piotrzarycki nice work! wondering if you are able to make the updates? |
yep, will do it today :) |
@piotrzarycki just a friendly reminder :) |
@piotrzarycki will you be able to look at this? |
@piotrzarycki if you can't finish this up no worries. just let us know. :) |
Hi, sorry, i forgot about this task. |
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.
Nice updates! Looks good to me!
@piotrzarycki thanks for sticking with it! we appreciate it. :D |
Haha 😝. Yeah i hope next PR-s will be faster |
* upstream/master: (42 commits) docs: add Code of Conduct to project (GoogleChrome#11212) docs(readme): add related project: lighthouse-viewer (GoogleChrome#11250) core(font-size): remove deprecated DOM.getFlattenedDocument (GoogleChrome#11248) misc: fix typo in method name (GoogleChrome#11239) i18n: make double dollar validation less strict (GoogleChrome#10299) misc: rephrase comments to be more inclusive (GoogleChrome#11228) misc: tweak gcp scripts to work in google corp (GoogleChrome#11233) v6.2.0 (GoogleChrome#11232) report: correctly display CLS in budget table (GoogleChrome#11209) report: vertically center thumbnails (GoogleChrome#11220) i18n: import (GoogleChrome#11225) tests: istanbul ignore inpage function (GoogleChrome#11229) deps(snyk): update script to prune <0.0.0 and update snapshot (GoogleChrome#11223) core(stacks): timeout stack detection (GoogleChrome#11172) core(config): unsized-images to default (GoogleChrome#11217) core(image-elements): collect CSS sizing, ShadowRoot, & position (GoogleChrome#11188) core: add FormElements gatherer (GoogleChrome#11062) new_audit: report animations not run on compositor (GoogleChrome#11105) tests: update chromestatus expecatations (GoogleChrome#11221) deps: update dot-prop secondary dependency (GoogleChrome#11198) ...
fixes #10285