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

Add missing locale strings to y18n lookup #221

Merged
merged 5 commits into from
Aug 5, 2015
Merged

Add missing locale strings to y18n lookup #221

merged 5 commits into from
Aug 5, 2015

Conversation

nexdrew
Copy link
Member

@nexdrew nexdrew commented Aug 3, 2015

➕ Further testing revealed the following strings used in index.js were missing from y18n lookup:

"Path to JSON config file": "Path to JSON config file",
"Show help": "Show help",
"Show version number": "Show version number"

This PR fixes that.

Note that the first commit takes the naive approach, which results in using the en version for 'Show help' in this scenario:

var argv = require('yargs').help('h').locale('pirate').argv

The second commit addresses this by deferring the y18n lookup until the whole usage content is generated, instead of doing it when the option is defined.

Also note that this PR does not attempt to address the problem of updating locale strings before the desired locale is set (if it even is a problem):

var argv = require('yargs')
  .help('h')
  .updateStrings({
    'Show help': 'Use this here help desc instead' // applies to default locale en
  })
  .locale('pirate') // changes locale and thus discards previously updated strings
  .argv

See related discussion in PR #220.

@@ -91,6 +91,11 @@ module.exports = function (yargs, y18n) {
wrap = cols
}

var deferY18nLookupPrefix = '__yargsString__:'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit weird, but like you say, also fairly elegant! :shipit:

@nexdrew
Copy link
Member Author

nexdrew commented Aug 4, 2015

@bcoe Just so I'm clear, :shipit: = ok to merge?

Do you want me to attempt a release once the es.json and fr.json PRs are merged, or is that something I should leave to you? I assume the general steps are something like:

  1. update CHANGELOG.md in master
  2. npm version
  3. git push && git push --tags
  4. npm publish --tag next
  5. test yargs@next
  6. npm dist-tag add yargs@x.y.z latest

@bcoe
Copy link
Member

bcoe commented Aug 5, 2015

:shipit: = ship it.

You've got the steps correct for the release, but let's wait for @zkat to update the Spanish translation before we release. I talked to her today she's on it :)

You can push the next release once we get things merged 👍 -- I think I'll have time for some yargs hacking this weekend, perhaps we can close a few more issues.

nexdrew pushed a commit that referenced this pull request Aug 5, 2015
Add missing locale strings to y18n lookup
@nexdrew nexdrew merged commit bca018e into yargs:master Aug 5, 2015
@nexdrew nexdrew deleted the missed-y18n-strings branch August 5, 2015 15:14
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.

2 participants