Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

Support dynamic locales #141

Merged
merged 1 commit into from
Aug 2, 2019

Conversation

lieut-data
Copy link
Collaborator

This tweaks the locale support in a couple of ways. First, we now locate locales in the root assets/i18n directory, resolved via GetBundlePath. This hardens the plugin against changes the server might make it how it unpacks the plugin, and simplifies the distribution of i18n resources for users.

Secondly, we now register all locales found in the assets/i18n directory. This means that adding support for new locales simply requires dropping in the file without any need to modify the code or even recompile.

Learned a bunch about the framework as part of making these changes, so thanks for the opportunity :)

This tweaks the locale support in a couple of ways. First, we now locate locales in the root `assets/i18n` directory, resolved via `GetBundlePath`. This hardens the plugin against changes the server might make it how it unpacks the plugin, and simplifies the distribution of i18n resources for users.

Secondly, we now register all locales found in the `assets/i18n` directory. This means that adding support for new locales simply requires dropping in the file without any need to modify the code or even recompile.
@lieut-data
Copy link
Collaborator Author

Perusing the code some more, I realize there are a bunch of func *En methods that perhaps have English assumptions built into them. Though I note they all use translated strings, and the non-en locales fall back to them.

Feel free to push back if the intent was to prevent other locales since it wouldn't really work!

i18nDirectory, found := fileutils.FindDir(*p.ServerConfig.PluginSettings.Directory + "/" + manifest.Id + "/server/dist/i18n/")
if !found {
return fmt.Errorf("unable to find i18n directory")
bundlePath, err := p.API.GetBundlePath()
Copy link
Owner

Choose a reason for hiding this comment

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

awesome!

if err := i18n.LoadTranslationFile(filepath.Join(i18nDirectory, filename)); err != nil {
return err
p.API.LogError("Failed to load translation file", "filename", filename, "err", err.Error())
continue
Copy link
Owner

Choose a reason for hiding this comment

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

better continuing here and logging, nice.

"github.com/stretchr/testify/require"
)

func TestTranslationsPreInit(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

thank you for adding test coverage on this. I learned some things from your style.

@scottleedavis
Copy link
Owner

func *En

I believe you are referring to funcs like createOccurrencesEN etc, which are meant to be english specific (parsing grammar strings), and will need specific handling for each supported local beyond the i18n json file.

e.g.

func (p *Plugin) CreateOccurrences(request *ReminderRequest) error {

	user, uErr := p.API.GetUserByUsername(request.Username)
	if uErr != nil {
		return uErr
	}
	_, locale := p.translation(user)

	switch locale {
	case "en":
		return p.createOccurrencesEN(request)
	default:
		return p.createOccurrencesEN(request)
	}

}

The only reason this is done because of the differences in ordering of grammar between languages. I am hoping to start spanish soon.

@scottleedavis
Copy link
Owner

Tested locally and works great, thank you @lieut-data !

@scottleedavis scottleedavis merged commit 8507681 into scottleedavis:master Aug 2, 2019
@lieut-data lieut-data deleted the dynamic-locales branch December 3, 2019 18:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants