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 i18n command line options #864

Merged
merged 7 commits into from
Jun 28, 2017
Merged

add i18n command line options #864

merged 7 commits into from
Jun 28, 2017

Conversation

charlierudolph
Copy link
Member

resolves #763

@mattwynne @danascheider I cleaned up the output and use different cli options then ruby in case you are interested.

@charlierudolph charlierudolph requested a review from jbpros June 24, 2017 22:12
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 98.332% when pulling b436422 on cr-i18n into 46a520b on master.

Copy link
Member

@jbpros jbpros left a comment

Choose a reason for hiding this comment

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

It looks great! Thanks @charlierudolph

@@ -50,6 +51,14 @@ export default class Cli {
async run() {
await validateInstall(this.cwd)
const configuration = await this.getConfiguration()
if (configuration.listI18nLanguages) {
Copy link
Member

Choose a reason for hiding this comment

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

what about passing stdout to the print* methods and let them do the actual writes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the current interface is simpler to unit test (not that I added unit tests, but just for future) since the methods just return strings. If we kept the current interface, should we rename the methods? Maybe just make them get* methods?

Copy link
Member

Choose a reason for hiding this comment

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

Good point! get* is better imo, then, yes.

@@ -36,6 +44,8 @@ export default class ArgvParser {
.option('--fail-fast', 'abort the run on first failure')
.option('-f, --format <TYPE[:PATH]>', 'specify the output format, optionally supply PATH to redirect formatter output (repeatable)', ArgvParser.collect, [])
.option('--format-options <JSON>', 'provide options for formatters (repeatable)', ArgvParser.mergeJson('--format-options'), {})
.option('--i18n-keywords <ISO 639-1>', 'list language keywords', ArgvParser.validateLanguage, '')
.option('--i18n-languages', 'list languages')
Copy link
Member

Choose a reason for hiding this comment

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

I like the name of those options. I find them better than in cucumber-rb.

Copy link

Choose a reason for hiding this comment

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

I like the name, too.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 98.332% when pulling 7ae9908 on cr-i18n into 46a520b on master.

@mattwynne
Copy link
Member

Thanks for the heads-up @charlierudolph

@lock
Copy link

lock bot commented Oct 24, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add the '--i18n' options for cli
5 participants