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

Add support for nested translation keys in i18n #124

Conversation

ain
Copy link

@ain ain commented Mar 20, 2014

@jonschlinkert
Copy link
Member

@ain this should probably get some tests before we consider merging.

@ain
Copy link
Author

ain commented Mar 20, 2014

@jonschlinkert I actually migrated this logic form PHP where I used it for a YAML-based translation engine, also key-based. And I pushed it to our production already.

Writing tests is a brilliant idea and should definitely be done.

I'll try to find some time and update the i18n tests too, probably next week.

@LaurentGoderre
Copy link
Contributor

This code changes loses the error when a string is not present. Is this on purpose? We would definitely want the build to fail if a string is missing.

@ain
Copy link
Author

ain commented Mar 21, 2014

@LaurentGoderre the scenario is, you don't always want to fail a build if key is missing. Also, if key is empty deliberately, it shouldn't fail. I'd propose bringing back the error, but making it possible with an additional parameter to suppress errors and provide empty string if the key is missing, e.g. {{i18n "key" language suppressFlag=false}}.

Additionally, maybe it's too much into i18n module's scope, but there are also scenarios in which you'd like to know if the key exists. Returning null allows that, e.g.:

{{#if (i18n "key")}}
  // standard behaviour
{{else}}
  // exceptional behaviour
{{#endif}}

So maybe it's even a good idea to go with null as default?

@doowb
Copy link
Member

doowb commented Mar 21, 2014

I like the addition of finding the value based on the parts of the key.
I think that instead of adding a suppressFlag, we add a default or fallback to the hash. If the default isn't set, then throw an error.

Throws error if foo is not found for US english

{{i18n "foo" language="en_US"}}

Uses default for foo if not found for US english

{{i18n "foo" language="en_US" default="bar"}}

If default="" allow that to return an empty string.

After these changes, I have an idea on how to make it work when the helper is nested too.

@jonschlinkert
Copy link
Member

why not just make this less verbose: {{i18n "foo" "bar" lang="en_US"}}? These mustaches will end up being huge lol.

For options like suppressFlag, I'm a fan of allowing these options to be set on the options hash and/or on the assemble options, or in a data file that passes the options to assemble. this is assuming that you don't want to have to define this on every single mustache. e.g.:

options: {
  i18n: {
    suppressFlag: false
  }
}

@ain
Copy link
Author

ain commented Mar 21, 2014

I like @doowb's suggestion, it's the best one, any objections?
With @jonschlinkert's suggestion we should be able to define global defaults, but still be able to define local in-template behaviour by helper arguments. That'd be the real-life-suitable solution.

@doowb
Copy link
Member

doowb commented Mar 21, 2014

These mustaches will end up being huge lol.

That's why they're called handlebars 😉

I like the combination of both. I was thinking of the global options too, but was going to add it as a second step. Also, I agree with @jonschlinkert that we need to make sure to have some tests around this.

@ain
Copy link
Author

ain commented Mar 21, 2014

👍

@jonschlinkert
Copy link
Member

closing since this lib has been refactored extensively

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.

Feature: support for nesting in i18n
4 participants