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

Added ES locale #220

Merged
merged 1 commit into from
Aug 5, 2015
Merged

Added ES locale #220

merged 1 commit into from
Aug 5, 2015

Conversation

zkat
Copy link
Contributor

@zkat zkat commented Aug 3, 2015

Here you go <3

@nexdrew
Copy link
Member

nexdrew commented Aug 3, 2015

This is great. Thank you so much @zkat!

One more quick thing, I noticed recently we missed a couple strings in the original locale implementation. Would you mind adding the following to es.json in anticipation of an upcoming fix, please?

"Show help": "<Your translation here>",
"Show version number": "<Your translation here>"

@nexdrew
Copy link
Member

nexdrew commented Aug 3, 2015

Found another one:

"Path to JSON config file": "<Translation needed>"

Might want to wait until I finish reviewing all missing i18n lookups. 😔

@nexdrew
Copy link
Member

nexdrew commented Aug 3, 2015

Blimey! I think that's all. Sorry for the hiccup!

"Path to JSON config file": "<Translation needed>",
"Show help": "<Your translation here>",
"Show version number": "<Your translation here>"

@zkat If you wouldn't mind adding these to es.json, I'd really appreciate it!

@zkat
Copy link
Contributor Author

zkat commented Aug 3, 2015

ping me again when you're done or tell @bcoe to poke me. :) I'll gladly
update things!

On Mon, Aug 3, 2015, 12:05 Andrew Goode notifications@github.com wrote:

Found another one:

"Path to JSON config file": ""

Might want to wait until I finish reviewing all missing i18n lookups. [image:
😔]


Reply to this email directly or view it on GitHub
#220 (comment).

kat

@bcoe
Copy link
Member

bcoe commented Aug 3, 2015

@zkat \o/ you're awesome. @nexdrew, perhaps you can sync up with @LoicMahieu too and get some additional French translation?

@bcoe bcoe mentioned this pull request Aug 3, 2015
@nexdrew
Copy link
Member

nexdrew commented Aug 3, 2015

@bcoe Sure. I was thinking I would attempt to add the French translation myself (via Google Translate, ick I know), and then see if @LoicMahieu would review the PR for us.

Considering these additional strings, I have an interesting problem for the fix and would like your opinion. The problem is that we need to defer the y18n lookup until after the locale has been set, so that we get consistent behavior b/w these 2 scenarios:

// locale defined BEFORE help option
var argv = require('yargs').locale('fr').help('h').argv
// locale defined AFTER help option
var argv = require('yargs').help('h').locale('fr').argv

Here are the options for implementation that I see:

  1. Defer the default description lookup until parsing (parseArgs() in index.js) to guarantee desired locale and description/custom strings have already been set
  2. Keep default description lookup in option methods (e.g. addHelpOpt()) and then attempt to manage/reset state if locale() or updateLocale() methods are subsequently called

Since the 2nd seems kludgy, I am leaning towards the 1st.

Thoughts?

@nexdrew
Copy link
Member

nexdrew commented Aug 3, 2015

Actually, if we don't call describe() when the option is added, then we run into problems maintaining "order of insertion" for the usage/help content.

Instead of managing state to keep track of what needs a y18n default description and the order in which it was inserted, I'm thinking it might be better to embed the state into the description itself as a special prefix (something like '__yargsString__:Show help') and then check for the prefix and do the y18n lookup when building the usage content in usage.help().

I know that sounds hacky, but it might simplify some things. I'm gonna give it a shot.

@nexdrew
Copy link
Member

nexdrew commented Aug 4, 2015

Attempt to address missing y18n lookups with PR #221.

@zkat Feel free to add the 3 strings mentioned above to es.json at your convenience. Thanks!

@zkat
Copy link
Contributor Author

zkat commented Aug 5, 2015

@nexdrew @bcoe all done. Any others you'd like?

@nexdrew nexdrew merged commit 38747df into yargs:master Aug 5, 2015
@nexdrew
Copy link
Member

nexdrew commented Aug 5, 2015

That's all for now, thanks @zkat!

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.

3 participants