Skip to content
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.

Sign up for GitHub

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

Issue 29 - Handle multiple YML files for translations #88

Closed
wants to merge 7 commits into from
Closed

Issue 29 - Handle multiple YML files for translations #88

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be01b27
refactor type TranslationMap
amerharb May 25, 2024
81f790f
save source file name
amerharb May 25, 2024
4e4f673
fix unit test ProjectStore.spec.ts
amerharb May 25, 2024
f8bb20c
quick fix for inner file id
amerharb May 25, 2024
8f6d9b0
add -1
amerharb May 25, 2024
4381987
fix getShortId function
amerharb May 25, 2024
c56b21f
Merge branch 'main' into issue-29-multi-yaml
amerharb Aug 17, 2024
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: 5 additions & 1 deletion webapp/src/Cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,18 @@

import { LanguageNotSupported } from '@/errors';
import { LyraProjectConfig } from '@/utils/lyraConfig';
import { MessageMap } from '@/utils/adapters';
import { ProjectStore } from '@/store/ProjectStore';
import { RepoGit } from '@/RepoGit';
import { ServerConfig } from '@/utils/serverConfig';
import { Store } from '@/store/Store';
import YamlTranslationAdapter from '@/utils/adapters/YamlTranslationAdapter';

export class Cache {
public static async getLanguage(projectName: string, lang: string) {
public static async getLanguage(
projectName: string,
lang: string,
): Promise<MessageMap> {
const serverProjectConfig =
await ServerConfig.getProjectConfig(projectName);
const repoGit = await RepoGit.getRepoGit(serverProjectConfig);
Expand Down
32 changes: 16 additions & 16 deletions webapp/src/RepoGit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { SimpleGitWrapper } from '@/utils/git/SimpleGitWrapper';
import { stringify } from 'yaml';
import { unflattenObject } from '@/utils/unflattenObject';
import { debug, info, warn } from '@/utils/log';
import { groupByFilename, TranslationMap } from '@/utils/adapters';
import { WriteLanguageFileError, WriteLanguageFileErrors } from '@/errors';

export class RepoGit {
Expand Down Expand Up @@ -143,27 +144,26 @@ export class RepoGit {
}

private async writeLangFiles(
languages: Record<string, Record<string, string>>,
languages: TranslationMap,
translationsPath: string,
): Promise<string[]> {
const paths: string[] = [];
const translateFilenameMap = groupByFilename(languages);
const result = await Promise.allSettled(
Object.keys(languages).map(async (lang) => {
const yamlPath = path.join(
translationsPath,
// TODO: what if language file were yaml not yml?
`${lang}.yml`,
);
const yamlOutput = stringify(unflattenObject(languages[lang]), {
doubleQuotedAsJSON: true,
singleQuote: true,
Object.values(translateFilenameMap).map((filenames) => {
Object.entries(filenames).forEach(async ([filename, messages]) => {
const yamlPath = path.join(translationsPath, filename);
const yamlOutput = stringify(unflattenObject(messages), {
doubleQuotedAsJSON: true,
singleQuote: true,
});
try {
await fsp.writeFile(yamlPath, yamlOutput);
} catch (e) {
throw new WriteLanguageFileError(yamlPath, e);
}
paths.push(yamlPath);
});
try {
await fsp.writeFile(yamlPath, yamlOutput);
} catch (e) {
throw new WriteLanguageFileError(yamlPath, e);
}
paths.push(yamlPath);
}),
);
if (result.some((r) => r.status === 'rejected')) {
Expand Down
4 changes: 3 additions & 1 deletion webapp/src/app/api/translations/[projectName]/[lang]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Cache } from '@/Cache';
import { dehydrateMessageMap } from '@/utils/adapters';
import {
LanguageNotFound,
LanguageNotSupported,
Expand All @@ -12,7 +13,8 @@ export async function GET(
) {
const { projectName, lang } = context.params;
try {
const translations = await Cache.getLanguage(projectName, lang);
const messageMap = await Cache.getLanguage(projectName, lang);
const translations = dehydrateMessageMap(messageMap);
return NextResponse.json({
lang,
translations,
Expand Down
39 changes: 23 additions & 16 deletions webapp/src/store/ProjectStore.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ describe('ProjectStore', () => {
getTranslations: async () => ({
de: {
'greeting.headline': {
sourceFile: '',
sourceFile: 'de-with_extra_text.yaml',
Comment on lines 21 to +23
Copy link
Collaborator

@WULCAN WULCAN Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A sourceFile under 'de' with a value not starting with 'de.' is unrealistic.

The first dot-separated segment of the last portion of the file's path is used as language identifier by the real YamlTranslationAdapter.

See also the test can update translations before getTranslations() below.

text: 'Hallo',
},
},
sv: {
'greeting.headline': {
sourceFile: '',
sourceFile: 'sv.yaml',
text: 'Hej',
},
},
Expand All @@ -34,7 +34,10 @@ describe('ProjectStore', () => {

const actual = await projectStore.getTranslations('de');
expect(actual).toEqual({
'greeting.headline': 'Hallo',
'greeting.headline': {
sourceFile: 'de-with_extra_text.yaml',
text: 'Hallo',
},
});
});

Expand All @@ -53,32 +56,30 @@ describe('ProjectStore', () => {
getTranslations: async () => ({
de: {
'greeting.headline': {
sourceFile: '',
sourceFile: 'anything',
Copy link
Collaborator

@WULCAN WULCAN Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A sourceFile under locale identifier de with a value not starting with de. is unrealistic.

The first dot-separated segment of the last portion of the file's path is used as locale identifier by the real YamlTranslationAdapter.

Suggested change
sourceFile: 'anything',
sourceFile: 'de.yaml',

text: 'Hallo',
},
},
}),
});

const before = await projectStore.getTranslations('de');
const beforeTranslate = await projectStore.getTranslations('de');
const before = beforeTranslate['greeting.headline'].text;
await projectStore.updateTranslation('de', 'greeting.headline', 'Hallo!');
const after = await projectStore.getTranslations('de');
const afterTranslate = await projectStore.getTranslations('de');
const after = afterTranslate['greeting.headline'].text;

expect(before).toEqual({
'greeting.headline': 'Hallo',
});
expect(before).toEqual('Hallo');

expect(after).toEqual({
'greeting.headline': 'Hallo!',
});
expect(after).toEqual('Hallo!');
Comment on lines -68 to +75
Copy link
Collaborator

@WULCAN WULCAN Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test also covered that updateTranslation does not add any other properties to the objects in the maps returned by getTranslations. TypeScript already enforces that so there is no problem with losing that coverage here.

This is just a note from my review that I thought might be interesting. You can resolve the conversation when you have read it.

});

it('can update translations before getTranslations()', async () => {
const projectStore = new ProjectStore({
getTranslations: async () => ({
de: {
'greeting.headline': {
sourceFile: '',
sourceFile: 'anything',
text: 'Hallo',
},
},
Expand All @@ -89,7 +90,7 @@ describe('ProjectStore', () => {
const actual = await projectStore.getTranslations('de');

expect(actual).toEqual({
'greeting.headline': 'Hallo!',
'greeting.headline': { sourceFile: 'anything', text: 'Hallo!' },
});
});

Expand Down Expand Up @@ -146,10 +147,16 @@ describe('ProjectStore', () => {
const languages = await projectStore.getLanguageData();
expect(languages).toEqual({
de: {
'greeting.headline': 'Hallo',
'greeting.headline': {
sourceFile: '',
text: 'Hallo',
},
},
sv: {
'greeting.headline': 'Hej',
'greeting.headline': {
sourceFile: '',
text: 'Hej',
},
},
});
});
Expand Down
16 changes: 10 additions & 6 deletions webapp/src/store/ProjectStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { ITranslationAdapter, TranslationMap } from '@/utils/adapters';
import {
ITranslationAdapter,
MessageMap,
TranslationMap,
} from '@/utils/adapters';
import { LanguageNotFound, MessageNotFound } from '@/errors';

type StoreData = {
Expand All @@ -17,28 +21,28 @@ export class ProjectStore {
this.translationAdapter = translationAdapter;
}

async getLanguageData(): Promise<Record<string, Record<string, string>>> {
async getLanguageData(): Promise<TranslationMap> {
await this.initIfNecessary();

const output: Record<string, Record<string, string>> = {};
const output: TranslationMap = {};
for await (const lang of Object.keys(this.data.languages)) {
output[lang] = await this.getTranslations(lang);
}

return output;
}

async getTranslations(lang: string): Promise<Record<string, string>> {
async getTranslations(lang: string): Promise<MessageMap> {
await this.initIfNecessary();

const language = this.data.languages[lang];
if (!language) {
throw new LanguageNotFound(lang);
}

const output: Record<string, string> = {};
const output: MessageMap = {};
Object.entries(language).forEach(([key, value]) => {
output[key] = value.text;
output[key] = value;
});
Comment on lines +43 to 46
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cloning is no longer necessary as language is already a MessageMap.


return output;
Expand Down
68 changes: 60 additions & 8 deletions webapp/src/utils/adapters/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,19 @@ export type MessageData = {
}[];
};

export type MessageTranslation = {
sourceFile: string;
text: string;
};

export type MessageMap = Record<
string, // msg id
MessageTranslation
>;

export type TranslationMap = Record<
string,
Record<
string,
{
sourceFile: string;
text: string;
}
>
string, // lang
MessageMap
>;

export interface IMessageAdapter {
Expand All @@ -27,3 +31,51 @@ export interface IMessageAdapter {
export interface ITranslationAdapter {
getTranslations(): Promise<TranslationMap>;
}

/** convert { [id]: { file, texts } } to { [id]: text } */
export function dehydrateMessageMap(
translations: MessageMap,
): Record<string, string> {
const output: Record<string, string> = {};
Object.entries(translations).forEach(([id, { text }]) => {
output[id] = text;
});
return output;
}

type TranslationFilenameMap = Record<
string, // lang
Record<
string, // filename
Record<
string, // msg Id
string // text
>
>
>;

/** convert TranslateMap to { [lang]: { [filename]: {[msgId]: text} } */
export function groupByFilename(
translationMap: TranslationMap,
): TranslationFilenameMap {
const output: TranslationFilenameMap = {};
Object.entries(translationMap).forEach(([lang, messageMap]) => {
Object.entries(messageMap).forEach(([msgId, { text, sourceFile }]) => {
if (!output[lang]) {
output[lang] = {};
}
if (!output[lang][sourceFile]) {
output[lang][sourceFile] = {};
}
const shortId = getShortId(msgId, sourceFile);
output[lang][sourceFile][shortId] = text;
});
});
return output;
}

function getShortId(msgId: string, sourceFile: string): string {
const sourceFileDotCount = sourceFile.replace('/', '.').split('.').length - 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String.prototype.replace only replaces the first occurrence if pattern is a string.

For example getShortId('a.b.c.d', 'a/b/c/sv.yml') should return just 'd' but this implementation returns 'c.d'.

Fortunately, there is another function replaceAll that works as expected.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll

const msgIdArray = msgId.split('.').splice(sourceFileDotCount);
Comment on lines +78 to +79
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code overestimates how much of the message id comes from the path. The filename --- the last segment of the path, typically contains a dot, for example in 'sv.yml' but can in theory contain more dots too, for example 'sv.skånska.yaml'.

For example getShortId('a.b.c', 'a/sv.yml') should return 'b.c' but this implementation will return just 'c'.

return msgIdArray.join('.');
}
Comment on lines +77 to +81
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is complex enough that it should have a somewhat extensive test suite.