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

i18n: make double dollar validation less strict #10299

Merged

Conversation

piotrzarycki
Copy link
Contributor

@piotrzarycki piotrzarycki commented Feb 5, 2020

fixes #10285

@piotrzarycki piotrzarycki requested a review from a team as a code owner February 5, 2020 19:05
@piotrzarycki piotrzarycki requested review from paulirish and removed request for a team February 5, 2020 19:05
@connorjclark connorjclark changed the title i18n: change double dollar validation (#10285) i18n: make double dollar validation less strict Feb 6, 2020
Copy link
Member

@exterkamp exterkamp left a 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(/\$([^$]*?)\$/);
Copy link
Member

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]) {
Copy link
Member

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.

@paulirish
Copy link
Member

@piotrzarycki nice work! wondering if you are able to make the updates?

@piotrzarycki
Copy link
Contributor Author

@piotrzarycki nice work! wondering if you are able to make the updates?

yep, will do it today :)

@paulirish
Copy link
Member

@piotrzarycki just a friendly reminder :)

@paulirish
Copy link
Member

@piotrzarycki will you be able to look at this?

@paulirish
Copy link
Member

@piotrzarycki if you can't finish this up no worries. just let us know. :)

@piotrzarycki
Copy link
Contributor Author

Hi, sorry, i forgot about this task.
Will update today

Copy link
Member

@exterkamp exterkamp left a 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!

@paulirish paulirish merged commit 8b97821 into GoogleChrome:master Aug 8, 2020
@paulirish
Copy link
Member

@piotrzarycki thanks for sticking with it! we appreciate it. :D

@piotrzarycki
Copy link
Contributor Author

piotrzarycki commented Aug 8, 2020

Haha 😝. Yeah i hope next PR-s will be faster

radum added a commit to radum/lighthouse that referenced this pull request Aug 13, 2020
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double-Dollar assertion is overly aggressive
7 participants