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

implement locale fallback for all locales #5718

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SamMousa
Copy link
Contributor

This implements language fallbacks in a more generic approach.

Instead of picking the locale string collection at the start, we try the locales and their fallbacks in order searching for a specific string.

The goal is to allow supporting custom locales more easily:

  • Setting a survey locale to nl-something, which is unknown would result in strings from the nl translations.
  • Allow overriding just a single string for a locale, while other strings are taken from the base locale in a cascading fashion

The idea is that instead of picking a whole locale dictionary at the start, we just define all possible locales.
Imagine this situation:

defaultLocale = "en-US"
currentLocale = "de-DE"
requestedLocale = "nl-test"
requestedMessage= "otherItemText"

In the current version, requesting an unknown language will just use the currentLocale, and if that doesn't exist use the defaultLocale.
This PR changes this behavior to be more granular. It will check each dictionary:

  1. Check this.locales["nl-test"] if found return
  2. Check this.locales["nl"] if found return
  3. Check this.locales["de-DE"] if found return
  4. Check this.locales["de"] if found return
  5. Check this.locales["en-US"] if found return
  6. Check this.locales["en"] if found return
  7. If not found call this.onGetExternalString(...)

Marking as a draft so we can discuss the implementation. Tests still pass even though the implementation and behavior changed, this means that the tests were not thorough enough to detect this change in behavior.

@SamMousa SamMousa marked this pull request as ready for review March 3, 2023 10:04
@SamMousa
Copy link
Contributor Author

SamMousa commented Mar 3, 2023

Please review & provide me some feedback. The failing CI jobs are brittle and not related to any of the code changes!

@SamMousa
Copy link
Contributor Author

SamMousa commented Mar 8, 2023

@andrewtelnov apologies for the brutal ping. Could you check if this is something you would like in the core?

@SamMousa SamMousa changed the title wip: implement locale fallback for all locales implement locale fallback for all locales Mar 8, 2023
Copy link
Member

@andrewtelnov andrewtelnov left a comment

Choose a reason for hiding this comment

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

Why do you remove "getCurrentStrings"?
It can be used by somebody to get strings object by locale.

@andrewtelnov
Copy link
Member

@andrewtelnov apologies for the brutal ping. Could you check if this is something you would like in the core?

Hello Sam,
No problem at all.

Thank you,
Andrew

@SamMousa
Copy link
Contributor Author

SamMousa commented Mar 8, 2023

Why do you remove "getCurrentStrings"?

In this implementation a string is picked based on the best matching language, so it is no longer the case that a single object can be found for a language.

For example, the result of getCurrentStrings('nl-NL-informal') could just contain: {"completeText": "Informal thanks"}.
But the getString() function would actually work for all SurveyJS strings.

What is missing in the library is some documentation on what is considered part of the public API. I assumed this was not part of the public API and as it was no longer used internally I chose to remove it.
If you insist that it is part of the SJS public API then that would be indeed a breaking change; however due to the new way it works its result would already change so it is already a somewhat breaking change.

Note that since locales is already a public property the getCurrentStrings() is just a getter that offers no clear benefits except a fallback. But using it with the fallback doesn't actually work as expected with the new per string fallback mechanism.

@andrewtelnov
Copy link
Member

andrewtelnov commented Mar 8, 2023

@SamMousa I got an idea.
In my mind, it should work in the following way:

  1. Check this.locales["nl-test"] if found return
  2. Check this.locales["nl"] if found return
  3. Check default locale if found return
  4. Check this.locales["en"] locale if found return
    If not found call this.onGetExternalString(...)

I will need to write a unit test on this functionality.

Thank you,
Andrew

@andrewtelnov
Copy link
Member

andrewtelnov commented Mar 9, 2023

@SamMousa I am sorry, I have created my own PR. Here is the related issue.
I write code in TDD style and it is easy for me to create Unit test first and then write the code to make it works. I used your idea, but code is a litle bit different.
Please review PR, is it OK?

Thank you,
Andrew

@SamMousa
Copy link
Contributor Author

SamMousa commented Mar 9, 2023

Will do later today

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.

None yet

2 participants