i18n: align type names with docs #9461
Conversation
| if (artifacts.AppCacheManifest !== null) { | ||
| return { | ||
| score: 0, | ||
| displayValue: str_(UIStrings.displayValue, {AppCacheManifest: artifacts.AppCacheManifest}), |
There was a problem hiding this comment.
tsc couldn't tell this would never be null
| @@ -178,7 +178,7 @@ class Canonical extends Audit { | |||
| if (!URL.rootDomainsMatch(canonicalURL, baseURL)) { | |||
| return { | |||
| score: 0, | |||
| explanation: str_(UIStrings.explanationDifferentDomain, {url: canonicalURL}), | |||
| explanation: str_(UIStrings.explanationDifferentDomain, {url: canonicalURL.href}), | |||
There was a problem hiding this comment.
tsc couldn't tell this would have been toString()ed
| * @property {string} content the string that will be substituted into a message | ||
| * @property {string} [example] an example (to assist translators) of what the content may be in the final string | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef LhlMessage |
There was a problem hiding this comment.
rather than having the LHL messages be types of CTC messages (since all the CTC message properties were optional except message), makes sense to split the two
| * @property {string} [description] a string used by translators to give context to the message | ||
| * @property {string} [meaning] an arbitrary strings used by translators to differentiate messages that have the same message | ||
| * @property {Record<string, ICUPlaceholderDefn>} [placeholders] a set of values that are to be replaced in a message | ||
| * @property {string} description a string used by translators to give context to the message |
There was a problem hiding this comment.
we require this so should be required
| @@ -50,7 +50,8 @@ function swapLocale(lhr, requestedLocale) { | |||
|
|
|||
| const locale = i18n.lookupLocale(requestedLocale); | |||
| const {icuMessagePaths} = lhr.i18n; | |||
| const missingIcuMessageIds = /** @type {string[]} */([]); | |||
| /** @type {string[]} */ | |||
There was a problem hiding this comment.
does this have any practical effect?
There was a problem hiding this comment.
does this have any practical effect?
no, just a cast is overkill and may cause a double take :)
| @@ -103,37 +105,38 @@ function computeDescription(ast, property, value, startRange) { | |||
| * | |||
| * @param {string} message | |||
| * @param {Record<string, string>} examples | |||
| * @return {ICUMessageDefn} | |||
| * @return {Pick<CtcMessage, 'message'|'placeholders'>} | |||
There was a problem hiding this comment.
| * @return {Pick<CtcMessage, 'message'|'placeholders'>} | |
| * @return {IncrementalCtc} |
right?
There was a problem hiding this comment.
right?
yeah, takes some changes but probably for the best
| @@ -430,7 +435,7 @@ function collectAllStringsInDir(dir, strings = {}) { | |||
|
|
|||
| const messageKey = `${relativePath} | ${key}`; | |||
|
|
|||
| /** @type {ICUMessageDefn} */ | |||
| /** @type {CtcMessage} */ | |||
| const icuDefn = { | |||
| @@ -172,14 +172,14 @@ describe('Convert Message to Placeholder', () => { | |||
| it('passthroughs a basic message unchanged', () => { | |||
| const message = 'Hello World.'; | |||
| const res = collect.convertMessageToPlaceholders(message, undefined); | |||
| expect(res).toEqual({message, placeholders: undefined}); | |||
| expect(res).toEqual({message, placeholders: {}}); | |||
There was a problem hiding this comment.
Arg. This is less strict again 😦 Now there isn't a great check for regressions unless someone looks at the .ctc.json files after generation. Not a showstopper, but I liked that this was caught in the test.
There was a problem hiding this comment.
Arg. This is less strict again 😦 Now there isn't a great check for regressions unless someone looks at the
.ctc.jsonfiles after generation. Not a showstopper, but I liked that this was caught in the test.
yeah, we can play with a way to make it caught, but this was part of my thinking on if we should check in en-US.ctc.json after all...
Follow up to #9114
undefinednonsense by tsc (i18n: placeholders #9114 (comment))Record<string, string|number>since that's all we accept