Fix bug with pluralization or interpolation of 'null' message + tests

[MichaelHoste]

Hi there,

This is a small pull request about a bug we are facing right now.
If we use YAML with this kind of keys:

nl:
  badges:
    counting:
      one: 
      other: 
      zero: 

it's transformed to this in the .js file:

badges: {
  counting: {
    one: null,
    other: null,
    zero: null,
  }
}

And, if we use I18n.t("badges.counting", {count: 2}), we have an JavaScript error Cannot read property 'match' of undefined.

We solved it by returning an empty string instead of triggering the error. Would it be an acceptable solution for you?

[PikachuEXE]

I think it should still throw an error unless option fallback is passed in
It's harder for developers to debug if not error is thrown (when they put no value in locale files accidently)

[MichaelHoste]

Hi @PikachuEXE,

I agree with you, it shouldn't return "" like in the pull request, but it shouldn't raise an exception either.

It should return something like [missing "nl.badges.counting.other" translation] to the user. Throwing a Javascript error is really not optimal since the application is then "broken" when a translation is missing. And it can happen quite frequently.

If I have this YAML structure:

nl:
  badges:
    title:
    counting:
      one: 
      other: 
      zero: 

Why should there be a difference between:

I18n.t('badges.title') that returns [missing "nl.badges.title" translation]

and

I18n.t('badges.counting', {count: 2}) that throws an exception.

What do you think?

[PikachuEXE]

I am OK with returning an error message when null value is encountered

[MichaelHoste]

Sorry @PikachuEXE but are you ok with returning a text message (like we suggested) or with raising an error message (like it is now). We are not sure about it :p

If you're ok with returning a text (error) message, we'll investigate the code and try to achieve that.

[PikachuEXE]

Sorry for being unclear :)
I am fine with returning error message in string like [missing "nl.badges.title" translation] instead of throwing an error
Which makes a key with null in value the same as not having the key in translations

[MichaelHoste]

Cool, we'll try to make something out of this :-)

[claytron]

Any movement on this PR? We have to manually prune this stuff out in our current workflow. Would be great to get this fixed up!

[PikachuEXE]

I am waiting for PR starter to change the PR

[MichaelHoste]

I forgot about this. I'll try to work on this issue soon (this week?) but I don't know if I'll find my way into the code to trigger a "missing translation" message. I'll keep you in touch.

@claytron In the meantime, you can apply my fix to avoid JS errors (keys with null values will be replaced by empty strings without any error).

[MichaelHoste]

I hope this fix will solve the issue without introducing new bugs.

@PikachuEXE please check here : https://github.com/fnando/i18n-js/pull/393/files#diff-e731291dab1a6ee00870b26bf608dbffL533

I didn't understand why "scope" was sometimes an object. I removed this part and always send the scope as a string. Do you see any problem with that?

[PikachuEXE]

That method is called from translate
And the scope is from input directly
I doubt translate accept object as scope (first argument)

[PikachuEXE]

I guess before you change it
scope in pluralize could contain "translation scope in string" or "looked up translation" (which might be an object)
Making the code a bit confusing

[PikachuEXE]

I think it looks fine right now.
Can you squash commits?

[MichaelHoste]

Hi @PikachuEXE, good news! Thanks for the review.

Sorry but I don't really know how to properly squash the commits. I had to merge your master in my branch and I don't know what to do with all these commits (maybe I should have used pull --rebase instead).

If you prefer I can recreate these changes in a new "clean" branch (there are not so many), but can't you chose to squash the commits when accepting the pull request?

[PikachuEXE]

OK I guess I can try that
Thanks