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

setCatalog - allow your app to write updated translations back to file #221

Merged
merged 1 commit into from
Apr 20, 2016

Conversation

adam-clarey
Copy link
Contributor

This simple addition allows your app to write an updated catalog back to file. Meaning all translation management can be handled directly in your map and doesn't require someone to manually edit each translation file

@mashpie
Copy link
Owner

mashpie commented Mar 15, 2016

simple and nice - but needs test coverage

@mashpie mashpie added this to the 0.9.0 - feature improvements milestone Mar 15, 2016
@adam-clarey
Copy link
Contributor Author

Is that something I need to do? How would it be achieved?

@mashpie
Copy link
Owner

mashpie commented Mar 17, 2016

it would help a lot if you provide a test to get that merged quick. If not, I will add tests to describe that use case directly after merge, but with patience ;)

@adam-clarey
Copy link
Contributor Author

Whats the quickest way to get this merged in? I've added this module as a dependency for our js CMS - Iris. https://github.com/CityWebConsultants/Iris

I just don't know what you're expecting in the way of tests. This change is so simple and minor that I can't see anyway of it causing problems. It should be a non-breaking addition.

@mashpie
Copy link
Owner

mashpie commented Mar 20, 2016

I won't discuss the need for tests here :) each feature should be describe by a test (https://github.com/mashpie/i18n-node/tree/master/test) to ensure it's proper functionality even after future changes.

Anyway, I will add tests for you as far as I can imagine your implementation goals. Next Release will be after vacation in about 2 or 3 weeks.

@mashpie mashpie merged commit 0a48e2d into mashpie:master Apr 20, 2016
@mashpie
Copy link
Owner

mashpie commented Apr 20, 2016

one things keeps me wondering: i18n configured with updateFiles: true always populates new phrases to catalogs. The write() method never writes anything with updateFiles: false, yet. So how do you add any locales without updateFiles: true? But setting updateFiles: true will add any new phrase and write back the whole locale like your setCatalog would do.

Your implementation of setCatalog does nothing than writing to filesystem, even without updating in memory locales. In addition it doesn't check for existence of parameters, so you might loose all of your translations when invoked with locale. I don't think this is intended, right?

@mashpie
Copy link
Owner

mashpie commented Apr 20, 2016

I assume this describes you use case, right?

TestScope.setCatalog({en: {'Hello World': 'new World'}}); // --> should write to fs and update in memory
TestScope.getLocale(); // --> 'en'
TestScope.getCatalog('en'); // --> {'Hello World': 'new World'}
TestScope.__('Hello World'); // --> 'new World'

but how about some edge cases?

TestScope.setCatalog({'Hello World': 'new World'}); // --> should update of current locale only?
TestScope.setCatalog(); // --> should return without action or clear all???

@mashpie
Copy link
Owner

mashpie commented Apr 20, 2016

one last thing: the signature of that method should fit it's "getter" https://github.com/mashpie/i18n-node#i18ngetcatalog

@mashpie mashpie modified the milestones: 0.9.0 - feature improvements, 0.8.2 Apr 20, 2016
@rajkissu
Copy link

I'm trying to add new translations on the fly via node-i18n for the languages my app needs, setCatalog seems like the best way to do it:

i18n.setCatalog({
  en: {'Hello World': 'Hello World'},
  ms: {'Hello World': 'Hai dunia'},
  ...
})

__('Hello World') => returns 'Hai dunia' if locale set to 'ms'

However, I see you have removed the setCatalog method @mashpie - what's the reason for this, if you don't mind me asking? Is there another way to do this, hence the removal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants