-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
misc: simplify proto npm scripts #6406
Conversation
@@ -61,14 +61,13 @@ | |||
"i18n:checks": "./lighthouse-core/scripts/i18n/assert-strings-collected.sh", | |||
"i18n:collect-strings": "node lighthouse-core/scripts/i18n/collect-strings.js", | |||
"update:sample-artifacts": "node lighthouse-core/scripts/update-report-fixtures.js -G", | |||
"update:sample-json": "yarn i18n:collect-strings && node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --throttling-method=devtools --output=json --output-path=./lighthouse-core/test/results/sample_v2.json && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing", | |||
"update:sample-json": "yarn i18n:collect-strings && node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --throttling-method=devtools --output=json --output-path=./lighthouse-core/test/results/sample_v2.json && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing && yarn compile-proto && yarn build-proto-roundtrip", |
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.
that's quite the line these days 😆
just thinking out loud below 💭
I guess the contributor thinking here is that if they already had to run update:sample-json
then they would've failed the build without compile-proto
so forcing protobuf in the path of this command isn't really a barrier. If they just needed string collection that would still run before all this junk failed anyhow. 👍
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.
^ yes. any meaningful changes require running both.
separately, can we bikeshed the script name?? :) how about update:goldens
?
@@ -61,14 +61,13 @@ | |||
"i18n:checks": "./lighthouse-core/scripts/i18n/assert-strings-collected.sh", | |||
"i18n:collect-strings": "node lighthouse-core/scripts/i18n/collect-strings.js", | |||
"update:sample-artifacts": "node lighthouse-core/scripts/update-report-fixtures.js -G", | |||
"update:sample-json": "yarn i18n:collect-strings && node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --throttling-method=devtools --output=json --output-path=./lighthouse-core/test/results/sample_v2.json && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing", | |||
"update:sample-json": "yarn i18n:collect-strings && node ./lighthouse-cli -A=./lighthouse-core/test/results/artifacts --throttling-method=devtools --output=json --output-path=./lighthouse-core/test/results/sample_v2.json && node lighthouse-core/scripts/cleanup-LHR-for-diff.js ./lighthouse-core/test/results/sample_v2.json --only-remove-timing && yarn compile-proto && yarn build-proto-roundtrip", |
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.
^ yes. any meaningful changes require running both.
separately, can we bikeshed the script name?? :) how about update:goldens
?
adds proto roundtrip update to
yarn update:sample-json
and updatesbuild-proto
tobuild-proto-roundtrip
(this is (1) and (2) of #6405)