Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/plural-translations-require-other.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof schema> = {
meta: {
code: 'MatchingTranslations',
name: 'Translation files should have the same keys',
Expand All @@ -20,7 +25,7 @@ export const MatchingTranslations: JSONCheckDefinition = {
},
type: SourceCodeType.JSON,
severity: Severity.ERROR,
schema: {},
schema,
targets: [],
},

Expand All @@ -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 {};
Expand Down Expand Up @@ -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);

Expand Down
1 change: 1 addition & 0 deletions packages/theme-check-node/configs/all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ LiquidSyntaxError:
MatchingTranslations:
enabled: true
severity: 0
requireOther: true
MaxFileSize:
enabled: true
severity: 0
Expand Down
1 change: 1 addition & 0 deletions packages/theme-check-node/configs/recommended.yml
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ LiquidSyntaxError:
MatchingTranslations:
enabled: true
severity: 0
requireOther: true
MaxFileSize:
enabled: true
severity: 0
Expand Down
27 changes: 27 additions & 0 deletions packages/theme-check-node/src/config/load-config.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import {
recommended,
Severity,
SourceCodeType,
check as runChecks,
toSourceCode,
} from '@shopify/theme-check-common';
import { NodeFileSystem } from '../NodeFileSystem';
import {
createMockConfigFile,
createMockNodeModule,
Expand All @@ -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) => {
Expand Down
Loading