-
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
core(i18n): always use english for logs #5727
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ const MessageParser = require('intl-messageformat-parser'); | |
const LOCALES = require('./locales'); | ||
|
||
const LH_ROOT = path.join(__dirname, '../../'); | ||
const MESSAGE_INSTANCE_ID_REGEX = /(.* \| .*) # (\d+)$/; | ||
|
||
(() => { | ||
// Node usually doesn't come with the locales we want built-in, so load the polyfill if we can. | ||
|
@@ -196,13 +197,41 @@ function createMessageInstanceIdFn(filename, fileStrings) { | |
return getMessageInstanceIdFn; | ||
} | ||
|
||
/** | ||
* @param {string} icuMessageIdOrRawString | ||
* @param {LH.Locale} [locale] | ||
* @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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. works for me, done |
||
resolveIcuMessageInstanceId(icuMessageIdOrRawString, locale).formattedString : | ||
icuMessageIdOrRawString; | ||
} | ||
|
||
/** | ||
* @param {string} icuMessageInstanceId | ||
* @param {LH.Locale} [locale] | ||
* @return {{icuMessageInstance: IcuMessageInstance, formattedString: string}} | ||
*/ | ||
function resolveIcuMessageInstanceId(icuMessageInstanceId, locale = 'en-US') { | ||
const matches = icuMessageInstanceId.match(MESSAGE_INSTANCE_ID_REGEX); | ||
if (!matches) throw new Error(`${icuMessageInstanceId} is not a valid message instance ID`); | ||
|
||
const [_, icuMessageId, icuMessageInstanceIndex] = matches; | ||
const icuMessageInstances = _icuMessageInstanceMap.get(icuMessageId) || []; | ||
const icuMessageInstance = icuMessageInstances[Number(icuMessageInstanceIndex)]; | ||
|
||
const {formattedString} = _formatIcuMessage(locale, icuMessageId, | ||
icuMessageInstance.icuMessage, icuMessageInstance.values); | ||
|
||
return {icuMessageInstance, formattedString}; | ||
} | ||
|
||
/** | ||
* @param {LH.Result} lhr | ||
* @param {LH.Locale} locale | ||
*/ | ||
function replaceIcuMessageInstanceIds(lhr, locale) { | ||
const MESSAGE_INSTANCE_ID_REGEX = /(.* \| .*) # (\d+)$/; | ||
|
||
/** | ||
* @param {*} objectInLHR | ||
* @param {LH.I18NMessages} icuMessagePaths | ||
|
@@ -216,11 +245,8 @@ function replaceIcuMessageInstanceIds(lhr, locale) { | |
|
||
// Check to see if the value in the LHR looks like a string reference. If it is, replace it. | ||
if (typeof value === 'string' && MESSAGE_INSTANCE_ID_REGEX.test(value)) { | ||
// @ts-ignore - Guaranteed to match from .test call above | ||
const [_, icuMessageId, icuMessageInstanceIndex] = value.match(MESSAGE_INSTANCE_ID_REGEX); | ||
const messageInstancesInLHR = icuMessagePaths[icuMessageId] || []; | ||
const icuMessageInstances = _icuMessageInstanceMap.get(icuMessageId) || []; | ||
const icuMessageInstance = icuMessageInstances[Number(icuMessageInstanceIndex)]; | ||
const {icuMessageInstance, formattedString} = resolveIcuMessageInstanceId(value, locale); | ||
const messageInstancesInLHR = icuMessagePaths[icuMessageInstance.icuMessageId] || []; | ||
const currentPathAsString = _formatPathAsString(currentPathInLHR); | ||
|
||
messageInstancesInLHR.push( | ||
|
@@ -229,11 +255,8 @@ function replaceIcuMessageInstanceIds(lhr, locale) { | |
currentPathAsString | ||
); | ||
|
||
const {formattedString} = _formatIcuMessage(locale, icuMessageId, | ||
icuMessageInstance.icuMessage, icuMessageInstance.values); | ||
|
||
objectInLHR[property] = formattedString; | ||
icuMessagePaths[icuMessageId] = messageInstancesInLHR; | ||
icuMessagePaths[icuMessageInstance.icuMessageId] = messageInstancesInLHR; | ||
} else { | ||
replaceInObject(value, icuMessagePaths, currentPathInLHR); | ||
} | ||
|
@@ -251,5 +274,7 @@ module.exports = { | |
getDefaultLocale, | ||
getRendererFormattedStrings, | ||
createMessageInstanceIdFn, | ||
getDisplayValue, | ||
resolveIcuMessageInstanceId, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. yeah sure if you'd prefer :) |
||
replaceIcuMessageInstanceIds, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. since
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(I have no strong opinions) |
||
|
||
log.log('status', status); | ||
let auditResult; | ||
|
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 :)