Fallback to language-only file if language_territory file DNE #4
Conversation
|
@nexdrew this is looking good, I'll have some feedback end of day. |
|
One question that comes to mind, rather than simply returning a locale file's path split on lookup |
| var locale = JSON.parse(fs.readFileSync('./test/locales/fr.json', 'utf-8')) | ||
| locale.banana.should.equal('banana') | ||
| return done(err) | ||
| }) | ||
| }) | ||
|
|
||
| it('does not write new word to language file if fallbackToLanguage is false', function (done) { |
There was a problem hiding this comment.
might reword this test slightly, it wasn't immediately obvious to me... perhaps:
writes word to missing locale file, if no fallback takes place
There was a problem hiding this comment.
Yeah, I struggled to name these tests appropriately. Your idea is better. I'll change it.
|
No huge complaints, I like that we're moving more logic into y18n rather than yargs. |
|
Regarding this:
I thought about doing this, but I decided against implicitly changing a value that the consumer is explicitly specifying. This behavior just seems wrong to me: var y18n = Y18n({
locale: 'en_CA'
})
y18n.getLocale() // 'en_CA' => correct
y18n.__('Hello')
y18n.getLocale() // 'en' => what?I view using the language-only file more as an interpretation or "best effort" to support the given locale. I can kinda see how this might cause confusion if the consumer was expecting a new file to be written, but in that case, the consumer should explicitly set I could easily be persuaded against this, but that is my rationale. Here's some other ideas for what we could possibly do differently:
I had another idea but lost it during distractions encountered while writing this up. I'm definitely open to whatever you think is best. My initial implementation is definitely not very elegant. I just think that most systems deal with locales in the 'ab_CD' format (and sometimes 'ab-CD') instead of the 'ab' format, and 'ab' is implied if 'ab_CD' is not directly supported. |
|
My other idea was to emit an event if we do change the locale value, but I'm not sure there's enough value in that to outweigh the cost. |
| var frJson = JSON.parse(fs.readFileSync('./test/locales/fr.json', 'utf-8')) | ||
| frJson.should.deep.equal({ | ||
| meow: 'le meow' | ||
| }) |
There was a problem hiding this comment.
I understand your confusion now - this doesn't make sense here. It should go in the callback function below. The point is to verify that the fr.json file remained untouched.
Sorry, I will fix.
|
|
what do you think, minor version bump?
|
Yeah, sounds good. I'll get 3.1.0 out tonight, at the latest. Might see if I can upgrade deps while I'm at it. At least |
|
@nexdrew awesome, thanks; once you have I'll rebase my pull on yargs. |
Fallback to language-only file if language_territory file DNE
Based on yargs PR 231, attempt to better handle "full locales" (e.g.
en_US) by allowing y18n to fallback to language-only file (e.g.en.json) if no file exists for the language_territory combination (e.g.en_US.json).The first commit adds the initial implementation, which is, admittedly, not very efficient since it potentially adds 2 additional file system queries (
fs.statSync()) per cached locale. Wasn't sure how to get around this without refactoring quite a bit of code. Note that I created a wrapper forfs.statSync()instead of just usingfs.existsSync()directly since the latter is documented as "will be deprecated".The second commit adds a new configuration option
fallbackToLanguageso consumers can turn off this functionality if they don't like it. It's on by default, so that yargs can take advantage of it without any code changes. Not crazy about the config property name.I'm also not especially crazy about splitting on
'_'to splice out the language from the given locale value, but it should probably do for now.Feedback welcomed!