feat: config import translations #602

tomasciccola · 2024-05-02T16:16:47Z

May fix #587

…onfigImportTranslations

tomasciccola · 2024-05-09T15:51:17Z

I've basically followed the pattern with the *fields and *presetiterators.
I had to change the preset iterator so that it also yields its name (which is actually the object key) since that is used in translations to reference it (so translations don't use the name field in presets but the key itself; i.e. "Boundary Line" is actually "boundary-line")

tomasciccola · 2024-05-09T15:53:36Z

Also, keen to read alternatives to having 4 nested loops. Using a reducer is an alternative but I don't think it'll necessarily be clearer code...

tomasciccola · 2024-05-09T15:56:43Z

there's a type of translation thats always empty called category. I don't know the use of it, but I'm skipping it - for now - and only importing 'fields' and 'presets'. We may want to update the default config (and config builder) in the future to remove this..

EvanHahn

I wonder if you could avoid the nested loops by defining functions that make it clearer what each step does. Something like:

for (let [lang, languageTranslations] of Object.entries(
  translationsFile
)) {
  yield* translationsForLanguage(lang, languageTranslations)
}

// ...

/**
 * @param {string} lang
 * @param {Record<ValidDocTypes, unknown>} languageTranslations
 * @returns {Iterable<{name: string, value:import('@mapeo/schema').TranslationValue}>}
 */
function* translationsForLanguage(lang, languageTranslations) {
  const { language: languageCode, region: regionCode } = parseBCP47(lang)
  if (!languageCode) throw new Error(`invalid translation language`)
  for (let [schemaNameRef, languageTranslationsForDocType] of Object.entries(
    languageTranslations
  )) {
    // TODO: Define this function...
    yield* translationsForDocType(
      languageCode,
      regionCode,
      schemaNameRef,
      languageTranslationsForDocType
    )
  }
}

Might be clearer? Not sure.

there's a type of translation thats always empty called category. I don't know the use of it, but I'm skipping it - for now - and only importing 'fields' and 'presets'. We may want to update the default config (and config builder) in the future to remove this..

I agree that it'd be useful to remove this from the config.

tests/config-import.js

src/mapeo-project.js

EvanHahn · 2024-05-13T14:28:24Z

src/config-import.js

+    )) {
+      if (!isRecord(translationByDocType))
+        throw new Error('invalid translation docType object')
+      if (!(docType === 'presets' || docType === 'fields')) continue


Should we error or warn in this case?

I think we should warn but maybe after we update the default config so that it doesn't have categories (I can leave a TODO comment that addressed this, but for know we just continue?)

src/config-import.js

EvanHahn · 2024-05-13T14:40:48Z

src/config-import.js

+              throw new Error(`invalid translation field`)
+            for (let [fieldRef, message] of Object.entries(fieldsToTranslate)) {
+              // we skip messages that aren't strings, for now
+              if (isRecord(message)) continue


When would message ever be a record?

It shouldn't actually, it was a misconception I had based on badly parsing the file...

Well, it wasn't a misconception, there are actually some messages that are objects. This happens when we are translating an options in a Field.
Options should be a {label:string, value:string|boolean|number|null}[].
This is imposed on mapeo/schema.
Recently we updated the default config since options was only a string[] (see here for some examples). The thing is that we didn't updated the translations accordingly.
So, I think what should happen is:

We should update translations of options in fields in the default config in the shape of {label: translatedValue, value: translatedValue}

When message is an object, then we should iterate over it and produce a translation with fieldRef of options[idx].label and options[idx].value for every option (following dot-prop notation).

I think this makes sense?

Makes sense to me!

PR updating translation options in fields -> digidem/mapeo-default-config#58

src/config-import.js

1. split *translation into individual functions (to avoid nested loops) 2. remove `parseTranslationsFile` 3. return value without `docIdRef` (since we don't know it at that point) 4. move warnings outside of `readConfig` (to have it accessible in outside functions) 5. push to warnings instead of throwing errors 6. remove unnecessary check of `message` being an object

…onfigImportTranslations

EvanHahn · 2024-05-14T18:22:59Z

Let me know when I should take another look at this (no rush).

…onfigImportTranslations

EvanHahn

LGTM other than my small comments.

EvanHahn · 2024-05-29T19:46:18Z

src/config-import.js

+/** @type {Error[]} */
+let warnings = []


I think I see why this got hoisted, but I don't love that this is a global. Is there a way to avoid doing something like this? Maybe by moving translationsForLanguage into *translations()?

If two configs are imported at the same time, these warnings will get squashed because readConfig is asynchronous. Could we move this "back in" to readConfig?

src/config-import.js

EvanHahn · 2024-05-29T19:52:26Z

src/config-import.js

+  for (const [schemaNameRef, languageTranslationsForDocType] of Object.entries(
+    languageTranslations
+  )) {
+    // TODO: push to warnings when removing categories from default config


What does this mean? And should we actually add to warnings in this case, uncommenting line 287?

src/config-import.js

tests/config-import.js

shapes we expect

* find{Presets,Translations}File are concurrent * add quotes in test Co-authored-by: Evan Hahn <me@evanhahn.com>

…onfigImportTranslations

…peo-core-next into feat/configImportTranslations

…onfigImportTranslations

EvanHahn

LGTM other than my small comment. Happy to pair if helpful.

EvanHahn · 2024-06-05T14:39:36Z

src/config-import.js

+/** @type {Error[]} */
+let warnings = []


If two configs are imported at the same time, these warnings will get squashed because readConfig is asynchronous. Could we move this "back in" to readConfig?

…a closure

…onfigImportTranslations

EvanHahn

LGTM! One small non-blocking question.

EvanHahn · 2024-06-05T18:26:12Z

src/config-import.js

+      schemaNameRef,
+      languageTranslationsForDocType,
+    ] of Object.entries(languageTranslations)) {
+      // TODO: remove categories check when removed from default config


This is still necessary even with the new default config, right?

initial implementation of importing translations

3213479

tomasciccola self-assigned this May 2, 2024

Tomás Ciccola added 8 commits May 2, 2024 14:28

integrate importing translations in mapeo-project

dc5c628

fix type error

9ac5c3c

rigirous filter of docType

b70a324

add test import translations

d8459cb

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

8592fc6

…onfigImportTranslations

return name from preset iterator so that translation can refer to it

9c53ace

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

e3e00e7

…onfigImportTranslations

remove solo test and log

f00f72f

tomasciccola marked this pull request as ready for review May 9, 2024 15:51

tomasciccola requested review from EvanHahn and gmaclennan May 9, 2024 15:52

EvanHahn removed the request for review from gmaclennan May 13, 2024 14:11

EvanHahn requested changes May 13, 2024

View reviewed changes

Tomás Ciccola added 4 commits May 14, 2024 11:46

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

2b919fb

…onfigImportTranslations

reset warnings value on each call to readConfig

787baf5

incomplete TODO message

bee7aa7

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

527d949

…onfigImportTranslations

tomasciccola requested a review from EvanHahn May 28, 2024 13:39

Tomás Ciccola added 5 commits May 28, 2024 11:24

refactor *translations

f1f7311

add case for translation message being an object

c0a22f9

fix type error

9be9e61

update default-config, fix logic when msg is object, fix test

cc1fb46

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

c0ec2e7

…onfigImportTranslations

Tomás Ciccola added 3 commits May 29, 2024 11:49

move parsing of message object to outside function

4561d5b

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

41b2213

…onfigImportTranslations

fix linting on mapeo-project

a3fe308

EvanHahn requested changes May 29, 2024

View reviewed changes

Tomás Ciccola and others added 6 commits May 30, 2024 12:30

type value as what we expect, add isMessageObject to translate only

20f5e39

shapes we expect

Apply suggestions from code review

e0a17ba

* find{Presets,Translations}File are concurrent * add quotes in test Co-authored-by: Evan Hahn <me@evanhahn.com>

String -> string on *presets signature

26557db

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

7e87d30

…onfigImportTranslations

isMessageObject -> isFieldOptions

8330167

Merge branch 'feat/configImportTranslations' of github.com:digidem/ma…

a496bb4

…peo-core-next into feat/configImportTranslations

tomasciccola requested a review from EvanHahn May 30, 2024 15:42

Tomás Ciccola added 3 commits May 30, 2024 13:59

fix type error on isFieldOptions

f2226d1

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

487cf15

…onfigImportTranslations

specific check for 'categories' schemaNameRef

82aa0dc

EvanHahn requested changes Jun 5, 2024

View reviewed changes

Tomás Ciccola added 3 commits June 5, 2024 13:04

move warnings inside readConfig make every translate generator …

02b2087

…a closure

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

d500bd5

…onfigImportTranslations

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

1b7fc2a

…onfigImportTranslations

EvanHahn approved these changes Jun 5, 2024

View reviewed changes

tomasciccola merged commit eeb87af into main Jun 5, 2024
7 checks passed

tomasciccola deleted the feat/configImportTranslations branch June 5, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: config import translations #602

feat: config import translations #602

tomasciccola commented May 2, 2024

tomasciccola commented May 9, 2024

tomasciccola commented May 9, 2024

tomasciccola commented May 9, 2024

EvanHahn left a comment •

edited

EvanHahn May 13, 2024

tomasciccola May 14, 2024

EvanHahn May 13, 2024

tomasciccola May 14, 2024

tomasciccola May 28, 2024

tomasciccola May 28, 2024

EvanHahn May 28, 2024

tomasciccola May 28, 2024

EvanHahn commented May 14, 2024

EvanHahn left a comment

EvanHahn May 29, 2024

EvanHahn Jun 5, 2024

EvanHahn May 29, 2024

EvanHahn left a comment

EvanHahn Jun 5, 2024

EvanHahn left a comment

EvanHahn Jun 5, 2024

feat: config import translations #602

feat: config import translations #602

Conversation

tomasciccola commented May 2, 2024

tomasciccola commented May 9, 2024

tomasciccola commented May 9, 2024

tomasciccola commented May 9, 2024

EvanHahn left a comment • edited

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

Choose a reason for hiding this comment

EvanHahn commented May 14, 2024

EvanHahn 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

EvanHahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanHahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanHahn left a comment •

edited