misc: refactor collect-strings.js to use glob and work on windows #9406

connorjclark · 2019-07-18T06:46:20Z

connorjclark · 2019-07-18T06:51:10Z

lighthouse-core/scripts/i18n/collect-strings.js

+  `${path.sep}.git`,
+  `${path.sep}scripts`,
+  `${path.sep}node_modules`,
+  `${path.sep}test${path.sep}`,


i kinda hate this, wb this instead

[ '/.git', '/scripts', '/node_modules', '/test/', '-test.js', '-renderer.js', ].map(str => str.replace(/\//g, path.sep));

Meh, this seems even more obtuse imo.

I actually like @connorjclark's suggestion, the readability of the paths is the important thing IMO, the fact that we want to support multiple OS is the secondary concern, maybe just add the

// Ensure we use the OS-specific path separator comment to the map line?

exterkamp

LGTM, works on windows, so that's nice.

exterkamp · 2019-07-18T07:06:52Z

lighthouse-core/scripts/i18n/collect-strings.js

+  `${path.sep}.git`,
+  `${path.sep}scripts`,
+  `${path.sep}node_modules`,
+  `${path.sep}test${path.sep}`,


Meh, this seems even more obtuse imo.

patrickhulce

LGTM % nit

lighthouse-core/scripts/i18n/collect-strings.js

patrickhulce · 2019-07-18T16:28:18Z

lighthouse-core/scripts/i18n/collect-strings.js

+  `${path.sep}.git`,
+  `${path.sep}scripts`,
+  `${path.sep}node_modules`,
+  `${path.sep}test${path.sep}`,


I actually like @connorjclark's suggestion, the readability of the paths is the important thing IMO, the fact that we want to support multiple OS is the secondary concern, maybe just add the

// Ensure we use the OS-specific path separator comment to the map line?

brendankenny · 2019-07-18T19:58:36Z

sooooo...what if we just switched to using glob (already in our devDeps)? Handling OS-specific file stuff, searching for *.js files unless they match this set of patterns, and recursive subdirectory search are all included out of the box

Co-Authored-By: Patrick Hulce <[email protected]>

googlebot · 2019-07-18T20:00:13Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only "I consent." in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

paulirish · 2019-07-18T20:12:16Z

sooooo...what if we just switched to using glob

+1. I was kinda surprised to see us doing recursive directory descent ourselves.

i guess it'd be something a la

glob.sync("**/*.js", {
	ignore: ignoredPathComponents
}, fn);

though we could also read in our .gitignore and give that to ignore. Would take care of most of these.

brendankenny · 2019-07-18T20:28:35Z

cwd would also need to be set to dir, and setting root to LH_ROOT should give the relativePath for free (which should be normalized to posix /)

brendankenny · 2019-07-18T20:31:23Z

running yarn i18n:checks in appveyor would keep this from regressing on windows, but that involves making assert-strings-collected.sh work cross platform, so that's maybe something for the back burner :)

…-strings

connorjclark · 2019-07-18T20:47:04Z

Updated. Will have to continue when I'm on a windows machine again. But:

ignore doesn't seem to work.
I don't see any affect using root when cwd is also used.

patrickhulce

in this file's defense, it was written several months before glob was added to our deps and traversal isn't that hard :P

patrickhulce · 2019-07-18T22:33:18Z

lighthouse-core/scripts/i18n/collect-strings.js

+  const files = glob.sync('**/*.js', {
+    cwd: dir,
+    // TODO: why doesn't this work ...
+    // ignore: ignoredPathComponents,


probably because our ignored patterns aren't globs yet :)

brendankenny · 2019-07-18T22:39:03Z

I don't see any affect using root when cwd is also used.

whoops, root is not what we want, never mind! :)

brendankenny · 2019-07-18T22:46:11Z

whoops, root is not what we want, never mind! :)

I think you actually want something like

glob.sync(path.posix.relative(LH_ROOT, dir) + '/**/*.js', {cwd: LH_ROOT, ignore: ignoreGlobs})

posix is important so it does the separators right (glob only takes /), and it needs to be relative (and not just ${dir}/**/*.js) so that it doesn't give absolute paths.

Maybe there's a better way, though.

googlebot · 2019-07-25T05:06:52Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

connorjclark · 2019-07-25T05:07:19Z

ready to gogogo

patrickhulce

I believe you have the approvals @connorjclark were you wanting a re-review? :)

patrickhulce · 2019-07-25T14:19:45Z

lighthouse-core/scripts/i18n/collect-strings.js

+    cwd: LH_ROOT,
+    ignore: ignoredPathComponents,
+  });
+  for (const relativePathToRoot of files) {


Suggested change

for (const relativePathToRoot of files) {

for (const relativePathFromRoot of files) {

or filePathRelativeToRoot? :)

relativeToRootPath to match absolutePath.

connorjclark · 2019-07-25T17:38:47Z

I believe you have the approvals @connorjclark were you wanting a re-review? :)

well it was 10PM and I just did a weird merge. wanted @exterkamp to stamp

googlebot · 2019-07-25T17:41:00Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

brendankenny

@exterkamp probably should take a look since there is a bit changing in here and he just spent so much quality time with collect-strings :)

brendankenny · 2019-07-25T17:46:51Z

lighthouse-core/scripts/i18n/collect-strings.js

  _processPlaceholderDirectIcu(icuDefn, examples);
-


Whitespace like this comes down to author preference and should really be discussed in the original PR rather than changed in the next commit (inviting style edit wars). Presumably @exterkamp preferred it and no one brought it up in the review, so unless there's a reason to change it, the default move should be to leave it unchanged.

+1 leave the spaces.

ok sorry bud

exterkamp

LGTM! Didn't test on Windows on the latest iteration, but I did before and it was good, and I will again later today.

exterkamp · 2019-07-25T20:20:27Z

lighthouse-core/scripts/i18n/collect-strings.js

  _processPlaceholderDirectIcu(icuDefn, examples);
-


+1 leave the spaces.

exterkamp · 2019-07-25T22:26:48Z

lighthouse-core/scripts/i18n/collect-strings.js

@@ -382,82 +379,77 @@ const seenStrings = new Map();
 let collisions = 0;

 /**
- * @param {string} dir
- * @param {Record<string, ICUMessageDefn>} strings
+ * @param {string} dir absolute path


Comment on this, something like

* Collects all lhl messsages defined in UIString vars from within Javascript files located in dir, * and converts them into ctc format.

exterkamp · 2019-07-25T22:27:38Z

lighthouse-core/scripts/i18n/collect-strings.js

- * @param {string} dir
- * @param {Record<string, ICUMessageDefn>} strings
+ * @param {string} dir absolute path
+ * @return {Record<string, ICUMessageDefn>}


Good idea with the returning, I like this better!

exterkamp · 2019-07-25T22:28:05Z

lighthouse-core/scripts/i18n/collect-strings.js

+              strings[seenId].meaning = strings[seenId].description;
+              collisions++;
+            }
+            collisions++;


+1 to @paulirish for keeping this global so this isn't a problem 😄

sorry i have no idea what your comment is referencing but this line of code looks like i screwed up a merge?

is it supposed to double count collisions

nvm looks good

googlebot · 2019-07-26T00:24:30Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

connorjclark · 2019-07-26T00:24:54Z

Didn't test on Windows on the latest iteration, but I did before and it was good, and I will again later today.

i tested it already btw.

mergin'

misc: fix collect-strings.js for windows

70088e1

googlebot added the cla: yes label Jul 18, 2019

connorjclark mentioned this pull request Jul 18, 2019

i18n: placeholders #9114

Merged

connorjclark commented Jul 18, 2019

View reviewed changes

connorjclark requested a review from exterkamp July 18, 2019 06:51

exterkamp approved these changes Jul 18, 2019

View reviewed changes

patrickhulce approved these changes Jul 18, 2019

View reviewed changes

Update lighthouse-core/scripts/i18n/collect-strings.js

5940f4f

Co-Authored-By: Patrick Hulce <[email protected]>

googlebot removed the cla: yes label Jul 18, 2019

vercel bot deployed to staging July 18, 2019 20:00 View deployment

googlebot added the cla: no label Jul 18, 2019

glob

5b7f6ac

connorjclark requested review from brendankenny and paulirish as code owners July 18, 2019 20:38

vercel bot temporarily deployed to staging July 18, 2019 20:38 Inactive

Merge remote-tracking branch 'origin/master' into windows-fix-collect…

aa2de69

…-strings

vercel bot deployed to staging July 18, 2019 20:39 View deployment

connorjclark changed the title ~~misc: fix collect-strings.js for windows~~ misc: refactor collect-strings.js to use glob and work on windows Jul 18, 2019

patrickhulce reviewed Jul 18, 2019

View reviewed changes

pwd: LH_ROOT

8530cc8

vercel bot deployed to staging July 18, 2019 23:01 View deployment

ignore

2986d64

googlebot added cla: no and removed cla: yes labels Jul 25, 2019

googlebot added cla: yes and removed cla: no labels Jul 25, 2019

patrickhulce reviewed Jul 25, 2019

View reviewed changes

relativeToRootPath

726af3b

vercel bot deployed to staging July 25, 2019 17:40 View deployment

googlebot added cla: no and removed cla: yes labels Jul 25, 2019

brendankenny reviewed Jul 25, 2019

View reviewed changes

exterkamp approved these changes Jul 25, 2019

View reviewed changes

comment

8cb8b59

vercel bot temporarily deployed to staging July 25, 2019 22:55 Inactive

\n

3e3a5f5

vercel bot deployed to staging July 25, 2019 22:56 View deployment

lint

c33a1ac

vercel bot deployed to staging July 25, 2019 23:53 View deployment

connorjclark added cla: yes and removed cla: no labels Jul 26, 2019

connorjclark merged commit 905e976 into master Jul 26, 2019

connorjclark deleted the windows-fix-collect-strings branch July 26, 2019 00:25

paulirish pushed a commit that referenced this pull request Nov 6, 2019

misc: refactor collect-strings to use glob and work on windows (#9406)

b8e78d0

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

pfried mentioned this pull request Aug 4, 2022

feature file resolution does not work on windows machines NordicSemiconductor/cloud-e2e-bdd-test-runner-js#601

Closed

	for (const relativePathToRoot of files) {
	for (const relativePathFromRoot of files) {

misc: refactor collect-strings.js to use glob and work on windows #9406

misc: refactor collect-strings.js to use glob and work on windows #9406

Conversation

connorjclark commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

brendankenny commented Jul 18, 2019

googlebot commented Jul 18, 2019

paulirish commented Jul 18, 2019

brendankenny commented Jul 18, 2019

brendankenny commented Jul 18, 2019

connorjclark commented Jul 18, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jul 18, 2019

brendankenny commented Jul 18, 2019

googlebot commented Jul 25, 2019

connorjclark commented Jul 25, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Jul 25, 2019

googlebot commented Jul 25, 2019

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jul 26, 2019

connorjclark commented Jul 26, 2019

patrickhulce Jul 18, 2019 •

edited

Loading

patrickhulce Jul 18, 2019 •

edited

Loading

brendankenny Jul 25, 2019 •

edited

Loading