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.

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: localize audit error strings #6812

Merged
merged 4 commits into from
Dec 18, 2018
Merged

i18n: localize audit error strings #6812

merged 4 commits into from
Dec 18, 2018

Conversation

exterkamp
Copy link
Member

Summary
Audit errors were adding the message code to the end of the errorMessage with parens which was destroying the ICU replacement. This adds defaultProperties to easier facilitate mutliple LHErrors using the same UIString to replace and use their own defaults that are constant per error.

Related Issues/PRs
fixes: #6804

lighthouse-core/lib/lh-error.js Show resolved Hide resolved
const errorMessage = err.friendlyMessage ?
`${err.friendlyMessage} (${err.message})` :
`Audit error: ${err.message}`;
const errorMessage = err.friendlyMessage ? err.friendlyMessage : err.message;
Copy link
Collaborator

Choose a reason for hiding this comment

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

so what errors do we start losing for the code for after this, just the speedline ones? or the page load ones too?

I agree there's not a strong need for our users to know the code, but having it somewhere in the LHR would be nice.

Copy link
Member Author

@exterkamp exterkamp Dec 14, 2018

Choose a reason for hiding this comment

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

I think we should follow the same pattern of splitting the speedline ones out into unique values.

lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved
lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved
lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved
lighthouse-core/lib/lh-error.js Outdated Show resolved Hide resolved
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

Nevermind all that. Let's do separate messages for these errors. :)

@exterkamp exterkamp changed the title i18n: handle i18n in audit errros i18n: split badTraceRecording into unique UIStrings Dec 14, 2018
@exterkamp
Copy link
Member Author

Okay, flip flopped again. I was about to split out all the speedline UIStrings, and then noticed all the pageLoadTookTooLong UIStrings which would also need to be split up afaik. So this seemed like it was getting ridiculous, so @brendankenny and I came to the compromise of providing the code of the Error to the ICU replacement by default, this seems to make sense since the code is always enumerated, and is all the replacements needed anyway at this point.

Thoughts?

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

sgtm

@brendankenny brendankenny changed the title i18n: split badTraceRecording into unique UIStrings i18n: localize audit error strings Dec 18, 2018
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM!

Two last nits. The comment one probably wouldn't be worth another edit round on its own, but we probably should have the parens actually as parens in the test regex.

lighthouse-core/test/runner-test.js Outdated Show resolved Hide resolved
lighthouse-core/lib/lh-error.js Show resolved Hide resolved
@paulirish paulirish added the 4.0 label Dec 18, 2018
@brendankenny brendankenny merged commit 4f4ec55 into master Dec 18, 2018
@brendankenny brendankenny deleted the i18n-error-fix branch December 18, 2018 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit errorMessage not i18n'd
4 participants