Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add i18n for LighthouseError messages #6457
i18n: add i18n for LighthouseError messages #6457
Changes from 19 commits
d1be64d
6799fab
3db8628
b9c4b4e
9b6062b
b0d369f
1481b08
cf8e569
df29d17
020056a
56e3daf
e764e2d
7a65d02
a6cd8bc
02765e4
a991732
879c333
345d3e4
9a87b04
9beb615
bdfcc4e
c56c83f
9857760
d1e1943
c6cfb3b
14ceeb2
972ddb5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
can drop the template string.
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.
optional style nit: a lot of these
new LHError()
lines could also fit on one line now. Was it intentional breaking them up over 4? I feel like they're more readable on one, but understand if you feel otherwiseThere 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.
Personally I like splitting complex constructors into lines, and with it all on one line it skirts the line of es-lint yelling that the lines are too long. I feel like 4 lines is more readable than crammed onto 1.
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.
🎉
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.
do these still need to be in string templates?
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.
maybe for some of these descriptions we should include for the translators the distinction of whether it was something weird that retrying should fix, or something that the user needs to fix and try again? I'm thinking a stock explanation.
This is an error message that was because of certain user conditions they must fix before retrying.
vs.
This is an error message that was because of irregularities with Lighthouse that should be fixed by trying again.
just spitballin'
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.
🎉
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.
I feel like we're going to forget to update these if they ever change, but I'm not sure what to do about it
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.
Well the current plan is that if we change them, then there will be a ton of errors thrown from str_ because it is missing the required substitution fields. But I'm not sure that's a long term strategy.
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.
we could maybe figure out something with typescript, but I can't think of a way without keeping a manual list of error codes that will require an extra field.
@exterkamp would you want to do that? or wait on it.
main issue with
is we need to make sure error cases are getting hit in unit tests and that the unit tests aren't testing for any throw, since
str_
errors would fit that billThis file was deleted.