diff --git a/.changeset/plural-translations-require-other.md b/.changeset/plural-translations-require-other.md new file mode 100644 index 000000000..4fc0d9c30 --- /dev/null +++ b/.changeset/plural-translations-require-other.md @@ -0,0 +1,6 @@ +--- +'@shopify/theme-check-common': minor +'@shopify/theme-check-node': minor +--- + +Require `other` in pluralized storefront default locale entries with `MatchingTranslations`. The new `requireOther` option defaults to `true`; set it to `false` to disable this requirement while keeping missing and extra translation diagnostics. Schema locales and plural categories in other languages are unchanged. Reports the offending entry without inventing a translation or offering an automatic fix. diff --git a/packages/theme-check-common/src/checks/matching-translations/index.spec.ts b/packages/theme-check-common/src/checks/matching-translations/index.spec.ts index efea1e4e7..8eb8b14c9 100644 --- a/packages/theme-check-common/src/checks/matching-translations/index.spec.ts +++ b/packages/theme-check-common/src/checks/matching-translations/index.spec.ts @@ -5,6 +5,130 @@ import { MatchingTranslations } from '../../checks/matching-translations/index'; const prettyJSON = (json: any) => JSON.stringify(json, null, 2); describe('Module: MatchingTranslations', async () => { + it('should require other in a pluralized default translation', async () => { + const theme = { + 'locales/en.default.json': JSON.stringify({ items: { one: 'One item' } }), + }; + + const offenses = await check(theme, [MatchingTranslations]); + + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + message: "The pluralized translation 'items' is missing the 'other' key", + uri: 'file:///locales/en.default.json', + }); + expect(highlightedOffenses(theme, offenses)).to.deep.equal(['"items":{"one":"One item"}']); + expect(offenses[0].fix).to.be.undefined; + expect(offenses[0].suggest).to.be.undefined; + }); + + it.each(['zero', 'one', 'two', 'few', 'many'])( + 'should recognize the complete plural category %s', + async (category) => { + const offenses = await check( + { 'locales/en.default.json': JSON.stringify({ items: { [category]: 'Items' } }) }, + [MatchingTranslations], + ); + expect(offenses).to.have.length(1); + }, + ); + + it('should report each nested pluralized entry once with its source range', async () => { + const theme = { + 'locales/en.default.json': prettyJSON({ + cart: { items: { zero: 'No items', one: 'One item' } }, + many: { results: { few: 'A few results', many: 'Many results' } }, + }), + }; + const offenses = await check(theme, [MatchingTranslations]); + + expect(offenses).to.have.length(2); + for (const path of ['cart.items', 'many.results']) { + expect(offenses).to.containOffense({ + message: `The pluralized translation '${path}' is missing the 'other' key`, + uri: 'file:///locales/en.default.json', + }); + } + expect(highlightedOffenses(theme, offenses)).to.deep.equal([ + '"items": {\n "zero": "No items",\n "one": "One item"\n }', + '"results": {\n "few": "A few results",\n "many": "Many results"\n }', + ]); + }); + + it.each([ + ['other alone', { items: { other: '{{ count }} items' } }], + ['other with singular', { items: { one: 'One item', other: '{{ count }} items' } }], + ['ordinary strings', { title: 'Items' }], + ['empty objects', { items: {} }], + ['ordinary namespaces', { items: { title: 'Items', description: 'Your items' } }], + [ + 'category-like suffixes', + { items: { phone: 'Phone', someone: 'Someone', another: 'Another' } }, + ], + ['mixed category and ordinary keys', { items: { one: 'One', title: 'Items' } }], + ['category ancestors', { one: { many: { title: 'Items' } } }], + ['category keys with object values', { items: { one: { title: 'One' } } }], + ['nonstring values', { items: { one: 1, few: null, many: true } }], + ['arrays', { items: [{ count: { one: 'One' } }] }], + [ + 'external namespaces', + { shopify: { items: { one: 'One' } }, customer_accounts: { one: 'One' } }, + ], + ])('should not infer a missing other for %s', async (_name, translations) => { + const offenses = await check({ 'locales/en.default.json': JSON.stringify(translations) }, [ + MatchingTranslations, + ]); + expect(offenses).to.have.length(0); + }); + + it.each([ + 'locales/fr.json', + 'locales/en.default.schema.json', + 'locales/fr.schema.json', + 'assets/en.default.json', + 'config/settings_data.json', + ])('should not require other in %s', async (file) => { + const offenses = await check({ [file]: JSON.stringify({ items: { one: 'One item' } }) }, [ + MatchingTranslations, + ]); + expect(offenses).to.have.length(0); + }); + + it.each(['}', '{"items":{"one":"One item"}', '[]', 'null', '"Items"'])( + 'should ignore malformed or non-object default translations: %s', + async (source) => { + const offenses = await check({ 'locales/en.default.json': source }, [MatchingTranslations]); + expect(offenses).to.have.length(0); + }, + ); + + it.each([true, false])( + 'should preserve missing and extra diagnostics with requireOther: %s', + async (requireOther) => { + const offenses = await check( + { + 'locales/en.default.json': JSON.stringify({ items: { one: 'One item' }, title: 'Items' }), + 'locales/fr.json': JSON.stringify({ + items: { few: 'Quelques articles' }, + extra: 'Extra', + }), + }, + [MatchingTranslations], + {}, + { MatchingTranslations: { enabled: true, requireOther } }, + ); + expect(offenses).to.have.length(requireOther ? 3 : 2); + expect(offenses).to.containOffense({ + message: "The translation for 'title' is missing", + uri: 'file:///locales/fr.json', + }); + expect(offenses).to.containOffense({ + message: "A default translation for 'extra' does not exist", + uri: 'file:///locales/fr.json', + }); + }, + ); + it('should report offenses when the translation file is missing a key', async () => { for (const prefix of ['', '.schema']) { const theme = { diff --git a/packages/theme-check-common/src/checks/matching-translations/index.ts b/packages/theme-check-common/src/checks/matching-translations/index.ts index 8d31472eb..ded998c17 100644 --- a/packages/theme-check-common/src/checks/matching-translations/index.ts +++ b/packages/theme-check-common/src/checks/matching-translations/index.ts @@ -5,11 +5,16 @@ import { Severity, SourceCodeType, PropertyNode, + SchemaProp, } from '../../types'; const PLURALIZATION_KEYS = new Set(['zero', 'one', 'two', 'few', 'many', 'other']); -export const MatchingTranslations: JSONCheckDefinition = { +const schema = { + requireOther: SchemaProp.boolean(true), +}; + +export const MatchingTranslations: JSONCheckDefinition = { meta: { code: 'MatchingTranslations', name: 'Translation files should have the same keys', @@ -20,7 +25,7 @@ export const MatchingTranslations: JSONCheckDefinition = { }, type: SourceCodeType.JSON, severity: Severity.ERROR, - schema: {}, + schema, targets: [], }, @@ -38,7 +43,7 @@ export const MatchingTranslations: JSONCheckDefinition = { fileUri.endsWith('.default.json') || fileUri.endsWith('.default.schema.json'); const isSchemaTranslationFile = fileUri.endsWith('.schema.json'); - if (!isLocaleFile || isDefaultTranslationsFile || ast instanceof Error) { + if (!isLocaleFile || ast instanceof Error) { // No need to lint a file that isn't a translation file, we return an // empty object as the check for those. return {}; @@ -79,6 +84,39 @@ export const MatchingTranslations: JSONCheckDefinition = { .join('.'); }; + if (isDefaultTranslationsFile) { + if (isSchemaTranslationFile || !context.settings.requireOther) return {}; + + // Validate pluralized entries locally, without comparing the default locale to itself. + return { + async Property(node, ancestors) { + const value = node.value; + if (value.type !== 'Object' || value.children.length === 0) return; + if (ancestors.some((ancestor) => ancestor.type === 'Array')) return; + if ( + !value.children.every( + (child) => + isPluralizationNode(child) && + child.value.type === 'Literal' && + typeof child.value.value === 'string', + ) + ) { + return; + } + if (value.children.some((child) => child.key.value === 'other')) return; + + const path = objectPath(ancestors.concat(node)); + if (isExternalPath(path) || path === 'shopify' || path === 'customer_accounts') return; + + context.report({ + message: `The pluralized translation '${path}' is missing the 'other' key`, + startIndex: node.loc.start.offset, + endIndex: node.loc.end.offset, + }); + }, + }; + } + const countCommonParts = (arrayA: string[], arrayB: string[]): number => { const minLength = Math.min(arrayA.length, arrayB.length); diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index e275c92de..43e972374 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -118,6 +118,7 @@ LiquidSyntaxError: MatchingTranslations: enabled: true severity: 0 + requireOther: true MaxFileSize: enabled: true severity: 0 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index 15ef418a3..7ac3e497c 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -96,6 +96,7 @@ LiquidSyntaxError: MatchingTranslations: enabled: true severity: 0 + requireOther: true MaxFileSize: enabled: true severity: 0 diff --git a/packages/theme-check-node/src/config/load-config.spec.ts b/packages/theme-check-node/src/config/load-config.spec.ts index bcd473bbd..d1b57f31f 100644 --- a/packages/theme-check-node/src/config/load-config.spec.ts +++ b/packages/theme-check-node/src/config/load-config.spec.ts @@ -9,7 +9,10 @@ import { recommended, Severity, SourceCodeType, + check as runChecks, + toSourceCode, } from '@shopify/theme-check-common'; +import { NodeFileSystem } from '../NodeFileSystem'; import { createMockConfigFile, createMockNodeModule, @@ -36,8 +39,32 @@ describe('Unit: loadConfig', () => { const config = await loadConfig(undefined, __dirname); expect(config.checks).to.eql(recommended); expect(config.context).to.eql('theme'); + expect(config.settings.MatchingTranslations!.requireOther).to.equal(true); }); + it.each([true, false])( + 'passes requireOther: %s from YAML to MatchingTranslations', + async (requireOther) => { + const configPath = await createMockConfigFile( + tempDir, + `extends: nothing\nMatchingTranslations:\n enabled: true\n requireOther: ${requireOther}\n`, + ); + const config = await loadConfig(configPath, tempDir); + const uri = URI.file(path.join(tempDir, 'locales/en.default.json')).toString(); + const source = toSourceCode(uri, JSON.stringify({ items: { one: 'One item' } }))!; + const offenses = await runChecks([source], config, { fs: NodeFileSystem }); + + expect(offenses).to.have.length(requireOther ? 1 : 0); + if (requireOther) { + expect(offenses[0]).to.include({ + check: 'MatchingTranslations', + uri: source.uri, + message: "The pluralized translation 'items' is missing the 'other' key", + }); + } + }, + ); + describe.each(['shopify.extension.toml', 'shopify.app.toml'])( 'when the root contains a %s file', (fileName) => {