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.

Sign up for GitHub

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

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

Merged
merged 17 commits into from
Jul 26, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
70088e1
misc: fix collect-strings.js for windows
connorjclark Jul 18, 2019
5940f4f
Update lighthouse-core/scripts/i18n/collect-strings.js
connorjclark Jul 18, 2019
5b7f6ac
glob
connorjclark Jul 18, 2019
aa2de69
Merge remote-tracking branch 'origin/master' into windows-fix-collect…
connorjclark Jul 18, 2019
8530cc8
pwd: LH_ROOT
connorjclark Jul 18, 2019
2986d64
ignore
connorjclark Jul 18, 2019
6d30674
windows compat
connorjclark Jul 19, 2019
3d6b326
fix
connorjclark Jul 20, 2019
c55a900
update
connorjclark Jul 20, 2019
8d74620
bool
connorjclark Jul 20, 2019
8ef266e
Merge remote-tracking branch 'origin/master' into windows-fix-collect…
connorjclark Jul 25, 2019
3b778bf
ws
connorjclark Jul 25, 2019
baf44d7
ret type
connorjclark Jul 25, 2019
726af3b
relativeToRootPath
connorjclark Jul 25, 2019
8cb8b59
comment
connorjclark Jul 25, 2019
3e3a5f5
\n
connorjclark Jul 25, 2019
c33a1ac
lint
connorjclark Jul 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions lighthouse-core/scripts/i18n/collect-strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ const UISTRINGS_REGEX = /UIStrings = (.|\s)*?\};\n/im;
*/

const ignoredPathComponents = [
'/.git',
'/scripts',
'/node_modules',
'/test/',
`${path.sep}.git`,
`${path.sep}scripts`,
`${path.sep}node_modules`,
`${path.sep}test${path.sep}`,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i kinda hate this, wb this instead

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, this seems even more obtuse imo.

Copy link
Collaborator

@patrickhulce patrickhulce Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

'-test.js',
'-renderer.js',
];
Expand Down Expand Up @@ -86,7 +86,10 @@ function collectAllStringsInDir(dir, strings = {}) {
const key = property.key.name;
const message = exportVars.UIStrings[key];
const description = computeDescription(ast, property, lastPropertyEndIndex);
strings[`${relativePath} | ${key}`] = {message, description};
const pathKey = relativePath
// Windows compat.
.replace(/\\/g, '/');
strings[`${pathKey} | ${key}`] = {message, description};
lastPropertyEndIndex = property.range[1];
}
}
Expand Down