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.

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

resolve #8444 - Allow nested objects in translations #8793

Merged
merged 10 commits into from Nov 17, 2020

Conversation

oplik0
Copy link
Contributor

@oplik0 oplik0 commented Oct 24, 2020

resolves #8444

Now if a string key is not found, translator will attempt to find the string in a nested object by splitting the key on . character.
You can put default string inside the object (used when you want to get translation with just the name, but also have other properties under dots - patter I've seen in some places in core) in a property named the same as the original key, or an empty string.
for example:

{
  "test": {
    "": "test default"
  }
}

will return "test default" if the key is just "test". Same with

{
  "test": {
    "test": "test default"
  }
}

Translations will still correctly return the key and log a missing translation if the key is not in the object, obviously.

@julianlam
Copy link
Member

Well, this is wonderful! It'll work great with plugins... In core, however, we can't refactor the files unless Transifex supports nested objects, so some research is needed on that front.

What's the delimiter character, /?

@julianlam
Copy link
Member

Ah, I believe I suggested . in the original issue.

@oplik0
Copy link
Contributor Author

oplik0 commented Oct 25, 2020

Yes, I followed the original issue - the behavior is the same as it is now with core files, however if there is no string key (with . included in the string) the translator will now split the string on . characters and try to find the translation in nested objects.

However, after checking transifex documentation, it seems that it should support nested keys. I assume the format NodeBB uses is JSON with ICU Plurals (or KEYVALUEJSON) - as it's the only format that matches what I've seen in translation files. If so, then even the example there uses nested keys:

{
    "join": "Join",
    "nest": {
      "split": "Split",
      "another_nest": {
        "split": "Split"
      },
    "list": ["List", "Values", {"JSON": {"Embedded": "Document"}}]  
    },
    "files": "{count, plural, one {{count} file.} other {{count} files.}}"
}

So it seems to be supported :)

@CLAassistant
Copy link

CLAassistant commented Nov 2, 2020

CLA assistant check
All committers have signed the CLA.

@oplik0
Copy link
Contributor Author

oplik0 commented Nov 2, 2020

Err... Seems like I didn't do the rebase correctly :D Gotta fix it a bit...
edit: fixed

While I was at updating it to master where CI works, I decided to redo my implementation to be faster and more readable.

I decided to use a for loop instead of reduce or for ... of mostly because of performance. From my testing - in all browsers with the exception of Chrome (for some reason in Chrome for...of is a bit faster than normal for for me - it lags behind by a larger margin in all other chromium browsers though, and is the slowest method in Firefox) it's the fastest way to loop over an array and unlike reduce allows for easily terminating the loop early. So essentially now if there is something other than an object or string (at the end) the loop will terminate - that is it will return that key wasn't found.

There are still the two default keys at the end - if the loop ended with an object at the length of the deconstructed key, it will try the two defaults (name of the key part before it, or an empty string), but I swapped the order - now it's root key name first, empty string second. Since it seems more logical.
(so now on 'test': {'test': '1', '':'2'} looking just for test will return '1' instead of '2'

@oplik0
Copy link
Contributor Author

oplik0 commented Nov 15, 2020

Rebased it again to use GitHub Actions

@oplik0 oplik0 changed the base branch from master to v1.15.x November 17, 2020 12:55
@oplik0 oplik0 changed the base branch from v1.15.x to master November 17, 2020 12:55
@oplik0
Copy link
Contributor Author

oplik0 commented Nov 17, 2020

Seems like GitHub (still) doesn't track changes to base branch, so I had to change the base to something else and back for it to check and see that I only wanted to merge my commits and not everything between the previous rebase and the latest one

@barisusakli barisusakli merged commit 6e43086 into NodeBB:master Nov 17, 2020
@barisusakli barisusakli added this to the 1.15.2 milestone Nov 17, 2020
@oplik0 oplik0 deleted the issue-8444 branch November 17, 2020 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow language files to be written as nested js objects instead of a single level
4 participants