-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(i18n): always use english for logs #5727
Conversation
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.
love the extraction of resolveIcuMessageInstanceId
lighthouse-core/runner.js
Outdated
@@ -218,7 +218,7 @@ class Runner { | |||
*/ | |||
static async _runAudit(auditDefn, artifacts, settings, runWarnings) { | |||
const audit = auditDefn.implementation; | |||
const status = `Evaluating: ${audit.meta.title}`; | |||
const status = `Evaluating: ${i18n.getDisplayValue(audit.meta.title)}`; |
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.
since displayValue
is a thing i dont think we should reuse that name for this.
getOriginalMessage
?
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.
yeah I hesitated here, original message isn't quite right either. how bout
getFormatted
toFormatted
getFormattedValue
getFormattedString
toFormattedString
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.
getLocaleString
? reminiscent of toLocaleString
but different
(I have no strong opinions)
lighthouse-core/lib/i18n.js
Outdated
@@ -251,5 +274,7 @@ module.exports = { | |||
getDefaultLocale, | |||
getRendererFormattedStrings, | |||
createMessageInstanceIdFn, | |||
getDisplayValue, | |||
resolveIcuMessageInstanceId, |
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.
does this need to be exported and public? could keep it _private to the file.
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.
yeah sure if you'd prefer :)
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.
LGTM
lighthouse-core/lib/i18n.js
Outdated
* @return {string} | ||
*/ | ||
function getDisplayValue(icuMessageIdOrRawString, locale) { | ||
return MESSAGE_INSTANCE_ID_REGEX.test(icuMessageIdOrRawString) ? |
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.
style nit: with these really long lines the ternary gets pretty lost. IMO more readable as
if (MESSAGE_INSTANCE_ID_REGEX.test(icuMessageIdOrRawString)) {
return resolveIcuMessageInstanceId(icuMessageIdOrRawString, locale).formattedString;
}
return icuMessageIdOrRawString;
(at first I thought maybe the motivation was line length but this is in the clear :)
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.
works for me, done
lighthouse-core/lib/i18n.js
Outdated
* @param {LH.Locale} [locale] | ||
* @return {string} | ||
*/ | ||
function getDisplayValue(icuMessageIdOrRawString, locale) { |
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.
switch the test-utils
helper to use this?
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.
yup, that was the goal :)
fixes what @paulirish found in #5726