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 FR locale #222

Merged
merged 4 commits into from
Aug 6, 2015
Merged

Add missing locale strings to FR locale #222

merged 4 commits into from
Aug 6, 2015

Conversation

nexdrew
Copy link
Member

@nexdrew nexdrew commented Aug 4, 2015

🇫🇷 Some strings were found to be previously missing from y18n lookup; this adds them to fr.json with an attempted French translation:

"Path to JSON config file": "Chemin du fichier de configuration JSON",
"Show help": "Affiche de l'aide",
"Show version number": "Affiche le numéro de version"

See related discussion in PRs #220 and #221.

@LoicMahieu If you have time, I would greatly appreciate a quick review of these French translations. Thanks!

@LoicMahieu
Copy link
Contributor

Looks perfect 🚀
Just a remark not related specially to french language. It's about "JSON" or "json", we use the both "syntax" for describing the same thing. It would be good to harmonize ?
Personally I prefer the upper case.
/cc @bcoe

@nexdrew
Copy link
Member Author

nexdrew commented Aug 5, 2015

Thanks @LoicMahieu!

I agree there are some inconsistencies in the text that yargs outputs. Not only the treatment of "JSON" vs "json" (which should probably be upper-case, as you suggest), but also a couple error messages do not start with a capital letter.

I'll see what I can do to clean this up. Thanks again!

@nexdrew
Copy link
Member Author

nexdrew commented Aug 5, 2015

With commit 57af0ab, I made the following changes to address the inconsistencies in current locale strings:

  • Change "json" -> "JSON" for relevant y18n keys and values for all locales
  • Make sure all error messages start with a capital letter, including y18n keys and values for all locales

All tests so far look good. I will make sure the necessary changes are propagated to PR #224 for the Portuguese translation as well.

@nexdrew nexdrew mentioned this pull request Aug 5, 2015
@nexdrew
Copy link
Member Author

nexdrew commented Aug 5, 2015

@bcoe My last change will "break" user code that attempts to customize the following 2 error message keys via updateLocale():

  • "not enough arguments following: %s"
  • "invalid json config file: %s"

"Break" meaning their code will no longer override the default messages, since the y18n keys have changed.

It seems we would either:

  1. Stick with this change and bump major version when it is merged/released
  2. Revert the key change and live with the inconsistency between keys and values

What is your preference?

@bcoe
Copy link
Member

bcoe commented Aug 6, 2015

@nexdrew given how new this feature is, I think I'm comfortable making this a minor bump:

  1. I doubt many consuming libraries have customized these strings.
  2. I would argue slight copy edit could be considered a patch release -- I do think, as you've outlined, we should avoid it since it can be a hassle for upstream libraries.

Now that we have translations in for French and Spanish, feel free to cut a release 👍

Let's hold off on releasing Portuguese, @seldo would like to announce this translation as part of his talk at Brazil JS.

In other exciting news @sindresorhus is looking into creating a sync version of os-locale, so that we can automatically detect people's locale 💃

@nexdrew
Copy link
Member Author

nexdrew commented Aug 6, 2015

@bcoe Sounds good. I'll merge this as is and cut a 3.18.0 release today.

nexdrew pushed a commit that referenced this pull request Aug 6, 2015
Add missing locale strings to FR locale
@nexdrew nexdrew merged commit b42a949 into yargs:master Aug 6, 2015
@nexdrew nexdrew deleted the fr-missing-strings branch August 6, 2015 19:36
@nexdrew
Copy link
Member Author

nexdrew commented Aug 6, 2015

yargs@next now points to 3.18.0 😬

@nexdrew
Copy link
Member Author

nexdrew commented Aug 7, 2015

Ran a bunch of tests without problem. 3.18.0 is now yargs@latest. 😎

@bcoe
Copy link
Member

bcoe commented Aug 7, 2015

@nexdrew \o/

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