[CCI] Fix type errors in i18n #3629

Nicksqain · 2023-03-17T16:25:21Z

Description

Corrects error types of error handlers.
Corrects type errors for i18n and its configs.
Setting the type of the context for each of the helper functions and the list.

Issues Resolved

#3027

Check List

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

ashwin-pc

I like this PR, great job! just a few small comments but mostly there 😄

ashwin-pc · 2023-03-17T20:07:04Z

src/dev/i18n/utils/verify_icu_message.ts

+  } 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;
+        }
+      }
    }
  }


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

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

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

ashwin-pc · 2023-03-17T20:08:57Z

src/dev/run_i18n_check.ts

-          task: ({ config }) =>
-            new Listr(extractDefaultMessages(config, srcPaths), { exitOnError: true }),
+          task: ({ config }) => {
+            return new Listr(extractDefaultMessages(config!, srcPaths), { exitOnError: true });


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

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

src/dev/run_i18n_check.ts

ashwin-pc · 2023-03-17T20:10:33Z

src/dev/run_i18n_integrate.ts

      {
        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 }),


same here, you dont need this change

ashwin-pc · 2023-03-17T20:12:27Z

src/dev/run_i18n_integrate.ts

-            config,
-            log,
-          });
+          if (config) {


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

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.

codecov-commenter · 2023-03-17T20:55:36Z

Codecov Report

Merging #3629 (efbcbdb) into main (e871aea) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3629      +/-   ##
==========================================
- Coverage   66.46%   66.39%   -0.07%     
==========================================
  Files        3205     3209       +4     
  Lines       61562    61633      +71     
  Branches     9497     9506       +9     
==========================================
+ Hits        40916    40924       +8     
- Misses      18375    18425      +50     
- Partials     2271     2284      +13

Flag	Coverage Δ
Linux	`66.39% <100.00%> (-0.01%)`	⬇️
Windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/dev/i18n/integrate_locale_files.ts	`76.81% <ø> (ø)`
src/dev/i18n/utils/verify_icu_message.ts	`80.00% <100.00%> (ø)`

... and 52 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

joshuarrrr · 2023-03-21T00:06:15Z

@Nicksqain When you add the next commits based on the review feedback, please also add an entry to the CHANGELOG.md file (under the "Refactoring" section).

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

ashwin-pc

Thanks for making the changes @Nicksqain. Just a few last comments and this PR should be good to merge :)

ashwin-pc · 2023-03-27T20:31:26Z

src/dev/run_i18n_extract.ts

@@ -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';


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

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

./i18n/tasks. Right?

ashwin-pc · 2023-03-27T20:33:25Z

src/dev/run_i18n_extract.ts

      {
        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 }),


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

ashwin-pc · 2023-03-27T20:36:38Z

src/dev/run_i18n_integrate.ts

-            config,
-            log,
-          });
+          if (config) {


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.

…to common source Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

ashwin-pc

👍 Awesome job on your pull request! Thanks for the contribution! 🎉

ashwin-pc · 2023-03-29T17:29:57Z

@Nicksqain I did notice however that typescript still complains about the verify_icu_message.ts file. Didnt want to block the PR because we can fix it later too, but just wanted to call that out.

Nicksqain · 2023-03-29T17:33:00Z

@ashwin-pc Where exactly is the typescript error?
Maybe I'm checking for errors in the wrong way.
But I have no warnings or errors

ashwin-pc · 2023-03-29T20:57:28Z

Nicksqain · 2023-03-30T05:42:37Z

@ashwin-pc

I created an issue for this problem #3606
There are two solutions, but the second one is not bad either.

1. We need to update dependencies for parser to solve this problem.
2. I can solve it here too by applying template literals by concatenating all parser.SyntaxError arguments. What do you think?

joshuarrrr · 2023-03-30T23:30:19Z

@Nicksqain Given #3606, I'd strongly prefer to update the deprecated dependency, so no need to account for it in this PR.

joshuarrrr

Thanks so much!

* Changes related to i18n configs * Added changes to missed commits and also added tasks Listr interface to common source * Update imports for other files that using ListrContext interface Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com> --------- Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit fd61f7a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* Changes related to i18n configs * Added changes to missed commits and also added tasks Listr interface to common source * Update imports for other files that using ListrContext interface Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com> --------- Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com> Signed-off-by: vagimeli <vagimeli@amazon.com>

* Changes related to i18n configs * Added changes to missed commits and also added tasks Listr interface to common source * Update imports for other files that using ListrContext interface Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com> --------- Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com> (cherry picked from commit fd61f7a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Changes related to i18n configs * Added changes to missed commits and also added tasks Listr interface to common source * Update imports for other files that using ListrContext interface Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com> --------- Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com> Signed-off-by: David Sinclair <david@sinclair.tech>

Nicksqain added 2 commits March 17, 2023 21:42

Changes related to i18n configs

00d016c

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

Changes related to the 'Check i18n configs' task

cdda27b

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

Nicksqain requested a review from a team as a code owner March 17, 2023 16:25

ashwin-pc added the OSCI Open Source Contributor Initiative label Mar 17, 2023

ashwin-pc reviewed Mar 17, 2023

View reviewed changes

ashwin-pc self-assigned this Mar 17, 2023

kavilla linked an issue Mar 20, 2023 that may be closed by this pull request

Fix type errors in i18n #3027

Closed

Nicksqain added 2 commits March 23, 2023 21:37

Corrected the deficiencies

cc6494b

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

Adding a changelog entry and adding a missing bracket

8b22e9b

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

Nicksqain requested a review from ashwin-pc March 24, 2023 12:08

ashwin-pc reviewed Mar 27, 2023

View reviewed changes

Added changes to missed commits and also added tasks Listr interface …

5b68cf6

…to common source Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

Nicksqain requested a review from ashwin-pc March 28, 2023 10:00

Nicksqain and others added 2 commits March 28, 2023 16:11

Update imports for other files that using ListrContext interface

efbcbdb

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

Merge branch 'main' into i18n-typescript-error

f9fef23

ashwin-pc approved these changes Mar 29, 2023

View reviewed changes

joshuarrrr approved these changes Mar 30, 2023

View reviewed changes

joshuarrrr merged commit fd61f7a into opensearch-project:main Mar 30, 2023

joshuarrrr added v2.7.0 backport 2.x labels Mar 30, 2023

opensearch-trigger-bot bot mentioned this pull request Mar 30, 2023

[Backport 2.x] [CCI] Fix type errors in i18n #3750

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCI] Fix type errors in i18n #3629

[CCI] Fix type errors in i18n #3629

Nicksqain commented Mar 17, 2023 •

edited

Loading

ashwin-pc left a comment

ashwin-pc Mar 17, 2023

Nicksqain Mar 18, 2023

ashwin-pc Mar 17, 2023

Nicksqain Mar 18, 2023

ashwin-pc Mar 17, 2023

ashwin-pc Mar 17, 2023

ashwin-pc Mar 27, 2023

codecov-commenter commented Mar 17, 2023 •

edited

Loading

joshuarrrr commented Mar 21, 2023

ashwin-pc left a comment

ashwin-pc Mar 27, 2023

Nicksqain Mar 28, 2023

Nicksqain Mar 28, 2023 •

edited

Loading

ashwin-pc Mar 27, 2023

ashwin-pc Mar 27, 2023

ashwin-pc left a comment

ashwin-pc commented Mar 29, 2023

Nicksqain commented Mar 29, 2023 •

edited

Loading

ashwin-pc commented Mar 29, 2023

Nicksqain commented Mar 30, 2023 •

edited

Loading

joshuarrrr commented Mar 30, 2023

joshuarrrr left a comment

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

[CCI] Fix type errors in i18n #3629

[CCI] Fix type errors in i18n #3629

Conversation

Nicksqain commented Mar 17, 2023 • edited Loading

Description

Issues Resolved

Check List

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 17, 2023 • edited Loading

Codecov Report

joshuarrrr commented Mar 21, 2023

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicksqain Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

ashwin-pc commented Mar 29, 2023

Nicksqain commented Mar 29, 2023 • edited Loading

ashwin-pc commented Mar 29, 2023

Nicksqain commented Mar 30, 2023 • edited Loading

joshuarrrr commented Mar 30, 2023

joshuarrrr left a comment

Choose a reason for hiding this comment

Nicksqain commented Mar 17, 2023 •

edited

Loading

codecov-commenter commented Mar 17, 2023 •

edited

Loading

Nicksqain Mar 28, 2023 •

edited

Loading

Nicksqain commented Mar 29, 2023 •

edited

Loading

Nicksqain commented Mar 30, 2023 •

edited

Loading