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

Jump to bottom

[CCI] Fix type errors in i18n #3629

Merged
merged 7 commits into from
Mar 30, 2023
Merged

[CCI] Fix type errors in i18n #3629

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
00d016c
Changes related to i18n configs
Nicksqain Mar 17, 2023
cdda27b
Changes related to the 'Check i18n configs' task
Nicksqain Mar 17, 2023
cc6494b
Corrected the deficiencies
Nicksqain Mar 23, 2023
8b22e9b
Adding a changelog entry and adding a missing bracket
Nicksqain Mar 23, 2023
5b68cf6
Added changes to missed commits and also added tasks Listr interface …
Nicksqain Mar 28, 2023
efbcbdb
Update imports for other files that using ListrContext interface
Nicksqain Mar 28, 2023
f9fef23
Merge branch 'main' into i18n-typescript-error
ashwin-pc Mar 29, 2023
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
2 changes: 1 addition & 1 deletion src/dev/i18n/integrate_locale_files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export function verifyMessages(
typeof message === 'string' ? message : message.text,
messageId
);
} catch (err) {
} catch (err: any) {
if (options.ignoreIncompatible) {
localizedMessagesMap.delete(messageId);
options.log.warning(`Incompatible translation ignored: ${err.message}`);
Expand Down
3 changes: 3 additions & 0 deletions src/dev/i18n/tasks/check_compatibility.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ export interface I18nFlags {
}

export function checkCompatibility(config: I18nConfig, flags: I18nFlags, log: ToolingLog) {
if (!config) {
throw new Error('Config is missing');
}
const { fix, ignoreIncompatible, ignoreUnused, ignoreMalformed, ignoreMissing } = flags;
return config.translations.map((translationsPath) => ({
task: async ({ messages }: { messages: Map<string, { message: string }> }) => {
Expand Down
6 changes: 3 additions & 3 deletions src/dev/i18n/tasks/check_configs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@
* specific language governing permissions and limitations
* under the License.
*/

import { ListrContext } from 'src/dev/run_i18n_check';
import { resolve, join } from 'path';
import { I18N_RC } from '../constants';
import { ErrorReporter, checkConfigNamespacePrefix, arrayify } from '..';
import { checkConfigNamespacePrefix, arrayify } from '..';

export function checkConfigs(additionalConfigPaths: string | string[] = []) {
const root = join(__dirname, '../../../../');
Expand All @@ -39,7 +39,7 @@ export function checkConfigs(additionalConfigPaths: string | string[] = []) {
const configPaths = [opensearchDashboardsRC, ...arrayify(additionalConfigPaths)];

return configPaths.map((configPath) => ({
task: async (context: { reporter: ErrorReporter }) => {
task: async (context: ListrContext) => {
try {
await checkConfigNamespacePrefix(configPath);
} catch (err) {
Expand Down
3 changes: 3 additions & 0 deletions src/dev/i18n/tasks/extract_default_translations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ import { createFailError } from '@osd/dev-utils';
import { ErrorReporter, extractMessagesFromPathToMap, filterConfigPaths, I18nConfig } from '..';

export function extractDefaultMessages(config: I18nConfig, inputPaths: string[]) {
if (!config) {
throw new Error('Config is missing');
}
const filteredPaths = filterConfigPaths(inputPaths, config) as string[];
if (filteredPaths.length === 0) {
throw createFailError(
Expand Down
14 changes: 6 additions & 8 deletions src/dev/i18n/tasks/extract_untracked_translations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,8 @@
*/

import { createFailError } from '@osd/dev-utils';
import {
I18nConfig,
matchEntriesWithExctractors,
normalizePath,
readFileAsync,
ErrorReporter,
} from '..';
import { ListrContext } from 'src/dev/run_i18n_check';
import { I18nConfig, matchEntriesWithExctractors, normalizePath, readFileAsync } from '..';

function filterEntries(entries: string[], exclude: string[]) {
return entries.filter((entry: string) =>
Expand Down Expand Up @@ -104,8 +99,11 @@ export async function extractUntrackedMessagesTask({
export function extractUntrackedMessages(inputPaths: string[]) {
return inputPaths.map((inputPath) => ({
title: `Checking untracked messages in ${inputPath}`,
task: async (context: { reporter: ErrorReporter; config: I18nConfig }) => {
task: async (context: ListrContext) => {
const { reporter, config } = context;
if (!config) {
throw new Error('Config is not defined');
}
const initialErrorsNumber = reporter.errors.length;
const result = await extractUntrackedMessagesTask({ path: inputPath, config, reporter });
if (reporter.errors.length === initialErrorsNumber) {
Expand Down
6 changes: 3 additions & 3 deletions src/dev/i18n/tasks/merge_configs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@
* specific language governing permissions and limitations
* under the License.
*/

import { ListrContext } from 'src/dev/run_i18n_check';
import { resolve, join } from 'path';
import { ErrorReporter, I18nConfig, assignConfigFromPath, arrayify } from '..';
import { assignConfigFromPath, arrayify } from '..';

export function mergeConfigs(additionalConfigPaths: string | string[] = []) {
const root = join(__dirname, '../../../../');
Expand All @@ -38,7 +38,7 @@ export function mergeConfigs(additionalConfigPaths: string | string[] = []) {
const configPaths = [opensearchDashboardsRC, ...arrayify(additionalConfigPaths)];

return configPaths.map((configPath) => ({
task: async (context: { reporter: ErrorReporter; config?: I18nConfig }) => {
task: async (context: ListrContext) => {
try {
context.config = await assignConfigFromPath(context.config, configPath);
} catch (err) {
Expand Down
24 changes: 14 additions & 10 deletions src/dev/i18n/utils/verify_icu_message.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,20 @@ export function verifyICUMessage(message: string) {
verifySelectFormatNode(node.format);
}
}
} catch (error) {
if (error.name === 'SyntaxError') {
const errorWithContext = createParserErrorMessage(message, {
loc: {
line: error.location.start.line,
column: error.location.start.column - 1,
},
message: error.message,
});
throw errorWithContext;
} catch (error: unknown) {
if (error instanceof Error) {
if (error instanceof parser.SyntaxError) {
if (error.name === 'SyntaxError') {
const errorWithContext = createParserErrorMessage(message, {
loc: {
line: error.location.start.line,
column: error.location.start.column - 1,
},
message: error.message,
});
throw errorWithContext;
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this nesting required? Cant you simply change the if condition to

if (error instanceof parser.SyntaxError && error.name === 'SyntaxError') {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if the error would be instance of parser.SyntaxError, so I left it so to get your opinion. I'll change it if it's right

}
23 changes: 15 additions & 8 deletions src/dev/run_i18n_check.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,14 @@ import {
mergeConfigs,
} from './i18n/tasks';

const skipOnNoTranslations = ({ config }: { config: I18nConfig }) =>
!config.translations.length && 'No translations found.';
export interface ListrContext {
config?: I18nConfig;
reporter: ErrorReporter;
messages: Map<string, { message: string }>;
}

const skipOnNoTranslations = (context: ListrContext) =>
!context.config?.translations?.length && 'No translations found.';
run(
async ({
flags: {
Expand Down Expand Up @@ -105,16 +110,17 @@ run(
{
title: 'Validating Default Messages',
skip: skipOnNoTranslations,
task: ({ config }) =>
new Listr(extractDefaultMessages(config, srcPaths), { exitOnError: true }),
task: ({ config }) => {
return new Listr(extractDefaultMessages(config!, srcPaths), { exitOnError: true });
Copy link
Member

Choose a reason for hiding this comment

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

You are already handling for a missing config value in the extractDefaultMessages task, you dont need this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can skipOnNoTranslations do a throw error if there is no config? So as not to create more checks. Although these functions will be used anyway and there is no other way to check if the config is in one place

},
},
{
title: 'Compatibility Checks',
skip: skipOnNoTranslations,
task: ({ config }) =>
task: ({ config }) => {
Nicksqain marked this conversation as resolved.
Show resolved Hide resolved
new Listr(
checkCompatibility(
config,
config!,
Nicksqain marked this conversation as resolved.
Show resolved Hide resolved
{
ignoreMalformed: !!ignoreMalformed,
ignoreIncompatible: !!ignoreIncompatible,
Expand All @@ -125,7 +131,8 @@ run(
log
),
{ exitOnError: true }
),
);
},
},
],
{
Expand All @@ -138,7 +145,7 @@ run(
const reporter = new ErrorReporter();
const messages: Map<string, { message: string }> = new Map();
await list.run({ messages, reporter });
} catch (error) {
} catch (error: ErrorReporter | Error) {
process.exitCode = 1;
if (error instanceof ErrorReporter) {
error.errors.forEach((e: string | Error) => log.error(e));
Expand Down
10 changes: 5 additions & 5 deletions src/dev/run_i18n_extract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { resolve } from 'path';
import { createFailError, run } from '@osd/dev-utils';
import { ErrorReporter, serializeToJson, serializeToJson5, writeFileAsync } from './i18n';
import { extractDefaultMessages, mergeConfigs } from './i18n/tasks';

import { ListrContext } from './run_i18n_check';
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This can come from a common source instead of another file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know how you import :(
What is considered a common source?

Copy link
Contributor Author

@Nicksqain Nicksqain Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

./i18n/tasks. Right?

run(
async ({
flags: {
Expand All @@ -59,19 +59,19 @@ run(
}
const srcPaths = Array().concat(path || ['./src', './packages']);

const list = new Listr([
const list = new Listr<ListrContext>([
{
title: 'Merging .i18nrc.json files',
task: () => new Listr(mergeConfigs(includeConfig), { exitOnError: true }),
},
{
title: 'Extracting Default Messages',
task: ({ config }) =>
new Listr(extractDefaultMessages(config, srcPaths), { exitOnError: true }),
new Listr(extractDefaultMessages(config!, srcPaths), { exitOnError: true }),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
new Listr(extractDefaultMessages(config!, srcPaths), { exitOnError: true }),
new Listr(extractDefaultMessages(config, srcPaths), { exitOnError: true }),

update extractDefaultMessages to accept an optional config just like checkCompatibility. This should never happen based on the order that they are called in, but should the order ever be swapped, the error will point us to the problem

},
{
title: 'Writing to file',
enabled: (ctx) => outputDir && ctx.messages.size,
enabled: (ctx) => Boolean(outputDir && ctx.messages.size),
task: async (ctx) => {
const sortedMessages = [...ctx.messages].sort(([key1], [key2]) =>
key1.localeCompare(key2)
Expand All @@ -90,7 +90,7 @@ run(
const reporter = new ErrorReporter();
const messages: Map<string, { message: string }> = new Map();
await list.run({ messages, reporter });
} catch (error) {
} catch (error: ErrorReporter | Error) {
process.exitCode = 1;
if (error instanceof ErrorReporter) {
error.errors.forEach((e: string | Error) => log.error(e));
Expand Down
31 changes: 17 additions & 14 deletions src/dev/run_i18n_integrate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import Listr from 'listr';
import { createFailError, run } from '@osd/dev-utils';
import { ErrorReporter, integrateLocaleFiles } from './i18n';
import { extractDefaultMessages, mergeConfigs } from './i18n/tasks';
import { ListrContext } from './run_i18n_check';

run(
async ({
Expand Down Expand Up @@ -90,30 +91,32 @@ run(

const srcPaths = Array().concat(path || ['./src', './packages']);

const list = new Listr([
const list = new Listr<ListrContext>([
{
title: 'Merging .i18nrc.json files',
task: () => new Listr(mergeConfigs(includeConfig), { exitOnError: true }),
},
{
title: 'Extracting Default Messages',
task: ({ config }) =>
new Listr(extractDefaultMessages(config, srcPaths), { exitOnError: true }),
new Listr(extractDefaultMessages(config!, srcPaths), { exitOnError: true }),
Copy link
Member

Choose a reason for hiding this comment

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

same here, you dont need this change

},
{
title: 'Integrating Locale File',
task: async ({ messages, config }) => {
await integrateLocaleFiles(messages, {
sourceFileName: source,
targetFileName: target,
dryRun,
ignoreIncompatible,
ignoreUnused,
ignoreMissing,
ignoreMalformed,
config,
log,
});
if (config) {
Copy link
Member

Choose a reason for hiding this comment

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

I like you pattern of throwing an error if config is missing instead of this

Copy link
Member

Choose a reason for hiding this comment

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

Looks like you missed this change. The reason i like throwing an error is because it makes it clear why the task failed. The config should never be empty, but if it is, based on this change this task will complete silently, but the other tasks like checkCompatibility will throw an error. And since we expect config to be defined in this task, I want to know if it did not run.

await integrateLocaleFiles(messages, {
sourceFileName: source,
targetFileName: target,
dryRun,
ignoreIncompatible,
ignoreUnused,
ignoreMissing,
ignoreMalformed,
config,
log,
});
}
},
},
]);
Expand All @@ -123,7 +126,7 @@ run(
const messages: Map<string, { message: string }> = new Map();
await list.run({ messages, reporter });
process.exitCode = 0;
} catch (error) {
} catch (error: ErrorReporter | Error) {
process.exitCode = 1;
if (error instanceof ErrorReporter) {
error.errors.forEach((e: string | Error) => log.error(e));
Expand Down