i18next / ng-i18next

translation for AngularJS using i18next
https://github.com/i18next/ng-i18next
MIT License
161 stars 54 forks source link

Must use different character for nsSeparator and keySeparator #126

Closed codeandcats closed 7 years ago

codeandcats commented 7 years ago

Using the same character for nsSeparator and keySeparator ('.') fails to translate keys within objects.

clients.json (CLIENTS namespace):

{
    "MAINTENANCE": {
        "DELETE_PROMPT": "Are you sure you want to delete this client?"
    }
}
i18next.t('CLIENTS.MAINTENANCE.DELETE_PROMPT') // nope!

Changing the namespace separator to ':' works as expected.

i18next.t('CLIENTS:MAINTENANCE.DELETE_PROMPT') // yaas!

Ideally you should be able to use the same character to separate the namespace from key paths, but if not, at the very least the documentation should make it very clear this is not an option. 😃

jamuhl commented 7 years ago

Sorry for not being clear. You might provide a PR for enabling same separators (https://github.com/i18next/i18next) or update the documentation (https://github.com/i18next/i18next.com) to be more specific.

codeandcats commented 7 years ago

Thanks Jamuhl, actually if neither of us feels like doing the work to allow use of the same namespace character, the next best simplest solution might be to error if init is called with the same character for both?

codeandcats commented 7 years ago

Or at least a console.warn(). What do you think?

jamuhl commented 7 years ago

I'm for adding something to the docs...this was yet never an issue in 5 years of i18next (or at least never an issue that was raised) - so i prefer not adding to code size.

codeandcats commented 7 years ago

Since you bring it up, I've noticed a few undocumented quirks if not bugs in the short time while using i18next and its little ecosystem, not all I have created issues for, but the ones that I have, have been met with this same attitude of "eh, there's a workaround therefore I don't care" (obviously I'm paraphrasing). Its been insightful to say the least.

jamuhl commented 7 years ago

Hm, not sure but seems you got some of my replies wrong - i'm sorry for that...i never meant to be rude or harsh. Might be as a non english native speaker i sometimes have bad luck in choosing the wrong words. Really not meant to do so...and sorry for that.

I could understand you had some frustrating moments using i18next - and even if you might not believe i care for that.

1) It's not about workarounds - at least this is what i believe - i18next comes with defaults and 1000 of options to bend it to the way you need it (with a price of having to leave the comfort zone - try and error) - i'm not sure you get this flexibility in any other i18n framework out there.

2) I take my time to write you back - because i care. I do take PR, to reply to endless question, looking through userland code - doing it for freem taking my time, taking time of my family and asking nothing in return. Ever wondered why so many repos are looking for new maintainers, stop work at all?

So coming back...if you got any issues using i18next - let us know - but accept if i think it's not that big of a deal...and if you think i'm wrong...show me i'm wrong...i have no problem in admitting i was wrong. But don't say i do not care...thank you.

codeandcats commented 7 years ago

I get where you are coming from. Its hard work having to maintain libraries when you have a job (that isn't working the library) and a family / home life.

I guess my expectation is a little higher on these repos over other open-source libraries because you have a commercial product that integrates with them and benefits from their improvement. So to ask the community to send you fixes for your product seems somewhat rich to me.

I also have a job and a family so I don't expect I'll be spending my time fixing your products.

I feel like I've been very fair in my criticism. I do get the general impression that if there is a workaround, even if it means the user of your library has to waste time and money, trial and erring to discover the right combination of the many different configuration options to make it sort of work, then you don't care to update the doco or code yourself and put the onus on the consumer to do it for you. You've stated this very clearly just now. I never said you don't care about anything at all ever, just that if there's a workaround then "...meh".

Anyways, thanks for your reply. I'll consider raising the other issues I've come across.

jamuhl commented 7 years ago

You tried locize.com 11 days ago - thank you for giving it a try. Sadly we failed to convince you that it is a good solution for you to use - but at least you liked i18next and decided to use that (could have been formatjs, polyglot or any other json formatted i18n solution - as all are supported in locize.com). So that's awesome.

I don't expect you to fix locize.

Locize is not i18next. Locize is a localization service that works with i18next, but also with polyglot, formatjs, and others. But undeniable, it's a way to support my contribution to i18next and a way to get commercial support for it.

i18next is opensource, no product, free forever, MIT - takes pull requests with zero value for locize. Will fix bugs not hitting customers of locize. Will respond to all your questions, will try to help as much as possible even to non customers.

But you're right...you got all that impressions. And it's my fault. You see a commercial product exploiting the community to fix their product.

I failed to make it visible - that locize is my way of a gittip - to support my time i invest in i18next. Almost a year ago there was a point were i stood in front of the decision to close down i18next - to much work, beside the one time contributions no other people wanting to take over or contribute more regulary, to much people complaining - less saying thank you, people taking, expecting for free - not contributing back. Like a lot of maintainers today i was exhausted, frustrated, disappointed. But i took the fight, rewrote i18next completely, redone docs, took additional weeks of my life to give back to the community. Created locize.com in the hope people would love the additional layer solving the localization process. But seems i failed again.

But anyway looking forward to solve the other upcoming issues in a more commercial way - and if the demand for that 'dot' in nsSeparator is so important i might give my weekend to enable that.

codeandcats commented 7 years ago

I was talking about adding a warning so not to waste other people's time (which I would have added for you btw given the simplicity), didn't ask you to give up a weekend, but yet you responded with "i prefer not adding to code size". LMFAO at code size, pull the other one mate.

Give yourself a medal mate.

jamuhl commented 7 years ago

Lets calm this a little down...i think in the end we both want the same. Just have some different point of views.

Please try i18next@6.1.0 - this should enable using . for both separators.

codeandcats commented 7 years ago

Jamuhl, I actually muted this thread after my last response but I came back to apologise.

I'm truely sorry for my part - that...escalated quickly. 😦

Ultimately you are right, it's a minor issue and I shouldn't have made a big deal out of it. By way of apology, I'll think about the other issues I've encountered and if I can fix them send you PRs.

Thanks for making the fix.

jamuhl commented 7 years ago

Sorry for coming back...please use i18next@6.1.1

I made a mistake by taking the first array item of array splitted by . granted as namespace. Which is unsafe.

i18next.t('CLIENTS.MAINTENANCE.DELETE_PROMPT') // ok as CLIENTS is namespace i18next.t('MAINTENANCE.DELETE_PROMPT') // would have taken MAINTENACE instead of defaultNS

this is fixed in 6.1.1 - we test the extracted NS against the list of namespaces in options (options.ns) if found it's used else we think the full string is just the key.

If you find any other issue regarding this feature or any other feature please let me know.

codeandcats commented 7 years ago

Hi Jamuhl,

Thanks for adding that change, I've had some time to test it this morning and although it worked for most translations, I noticed one of my namespaces, whichever namespace is first in the config, would consistently not get translated.

I narrowed it down to this line of code:

if (nsSeparator !== keySeparator || nsSeparator === keySeparator && this.options.ns.indexOf(parts[0])) namespaces = parts.shift();

I believe should be:

if (nsSeparator !== keySeparator || nsSeparator === keySeparator && this.options.ns.indexOf(parts[0]) > -1) namespaces = parts.shift();

I can submit a PR if you like, I'm just curious (for this and any future contributions) if you have any doco for contributing, e.g. install + build steps, conventions, etc.

jamuhl commented 7 years ago

argh...always the index ;)...just published i18next@6.1.2 fixing it - and added some tests for this feature.

There is no documentation for contribution. npm i to install build dependencies. npm test is all that should be done before submitting a PR. The PR will run through travis CI and coveralls anyway, so failing tests would show up in the PR anyway.