Skip to content

i18n: make double dollar validation less strict#10299

Merged
paulirish merged 2 commits into
GoogleChrome:masterfrom
piotrzarycki:fix/double_dollar_assertion
Aug 8, 2020
Merged

i18n: make double dollar validation less strict#10299
paulirish merged 2 commits into
GoogleChrome:masterfrom
piotrzarycki:fix/double_dollar_assertion

Conversation

@piotrzarycki

@piotrzarycki piotrzarycki commented Feb 5, 2020

Copy link
Copy Markdown
Contributor

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

@exterkamp exterkamp left a comment

Copy link
Copy Markdown
Member

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

function _ctcSanityChecks(icu) {
// '$$' i.e. "Double Dollar" is always invalid in ctc.
if (icu.message.match(/\$\$/)) {
const regexMatch = icu.message.match(/\$([^$]*?)\$/);

Copy link
Copy Markdown
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.

// '$$' i.e. "Double Dollar" is always invalid in ctc.
if (icu.message.match(/\$\$/)) {
const regexMatch = icu.message.match(/\$([^$]*?)\$/);
if (regexMatch && !regexMatch[1]) {

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@piotrzarycki

Copy link
Copy Markdown
Contributor Author

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

yep, will do it today :)

@paulirish

Copy link
Copy Markdown
Member

@piotrzarycki just a friendly reminder :)

@paulirish

Copy link
Copy Markdown
Member

@piotrzarycki will you be able to look at this?

@paulirish

Copy link
Copy Markdown
Member

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

@piotrzarycki

Copy link
Copy Markdown
Contributor Author

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

@exterkamp exterkamp left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@piotrzarycki

piotrzarycki commented Aug 8, 2020

Copy link
Copy Markdown
Contributor Author

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