-
Notifications
You must be signed in to change notification settings - Fork 894
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.
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
[i18n] Add additional translations for de-DE, es-ES, fr-FR, id-ID, it-IT, js-JP, ko-KR, pt-PT, tr-TR, zh-CN, and zh-TW #8788
Conversation
❌ Entry Too LongEntry is 119 characters long, which is 19 characters longer than the maximum allowed length of 100 characters. Please revise your entry to be within the maximum length. |
❌ Invalid PrefixInvalid description prefix. Found "". Expected "breaking", "deprecate", "feat", "fix", "infra", "doc", "chore", "refactor", "security", "skip", or "test". |
aacf405
to
ae5a04f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8788 +/- ##
==========================================
+ Coverage 60.75% 60.80% +0.04%
==========================================
Files 3798 3798
Lines 90690 90734 +44
Branches 14277 14290 +13
==========================================
+ Hits 55101 55169 +68
+ Misses 32090 32065 -25
- Partials 3499 3500 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -206,19 +215,94 @@ async function writeMessages( | |||
} | |||
} | |||
|
|||
function isSpreadableObject(value: unknown) { | |||
try { | |||
return { ...value }, true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think [...null]
results in error but {...null}
works, should it consider null
as non spreadable?
reading a bit more seems it's iterable vs enumerable, looking at usage i think it's fine since it's spreading into an object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
is an acceptable argument but thanks for thinking about it.
return false; // Found a key, so it is NOT empty | ||
} | ||
} | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have lodash for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are actively trying to phase out lodash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some tests for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing tests for this functionality are pretty inadequate. I have created an issue for it:
#8791
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some tests for the new functionality.
dbe4c40
to
49c049c
Compare
…with "i18n:integrate" Signed-off-by: Miki <[email protected]>
…-IT, ja-JP, ko-KR, pt-PT, tr-TR, zh-CN, and zh-TW Signed-off-by: Miki <[email protected]>
49c049c
to
63140cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for meeting patch coverage!
A cypress test is failing, but looks like that's been happening elsewhere
ciGroup10 is new group which currently has no tests. Hence merging this PR. |
…-IT, js-JP, ko-KR, pt-PT, tr-TR, zh-CN, and zh-TW (#8788) * [i18n-dev] Allow updating existing localization files when importing with "i18n:integrate" Signed-off-by: Miki <[email protected]> * [i18n] Add additional translations for de-DE, es-ES, fr-FR, id-ID, it-IT, ja-JP, ko-KR, pt-PT, tr-TR, zh-CN, and zh-TW Signed-off-by: Miki <[email protected]> * Changeset file for PR #8788 created/updated --------- Signed-off-by: Miki <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 87832f3) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…-IT, js-JP, ko-KR, pt-PT, tr-TR, zh-CN, and zh-TW (#8788) (#8821) * [i18n-dev] Allow updating existing localization files when importing with "i18n:integrate" * [i18n] Add additional translations for de-DE, es-ES, fr-FR, id-ID, it-IT, ja-JP, ko-KR, pt-PT, tr-TR, zh-CN, and zh-TW * Changeset file for PR #8788 created/updated --------- (cherry picked from commit 87832f3) Signed-off-by: Miki <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
[i18n] Add additional translations for de-DE, es-ES, fr-FR, id-ID, it-IT, ja-JP, ko-KR, pt-PT, tr-TR, zh-CN, and zh-TW
Changelog
Check List
yarn test:jest
yarn test:jest_integration