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

Print help only when there are no arguments #3617

Merged
merged 2 commits into from Jan 14, 2020
Merged

Conversation

yeonhoyoon
Copy link
Contributor

@yeonhoyoon yeonhoyoon commented Jan 8, 2020

knex cli should print help only when there are no arguments.
current implementation always prints help even when given correct arguments, such as knex migrate:status.
the correct way to check argument length is via process.argv.
reference: https://github.com/tj/commander.js/#outputhelpcb

@yeonhoyoon yeonhoyoon changed the title output help only when there are no arguments Print help only when there are no arguments Jan 8, 2020
@timorthi
Copy link
Contributor

timorthi commented Jan 8, 2020

+1 also running into this, looks like a regression from #3604

@kibertoad
Copy link
Collaborator

I'll take a look tomorrow, thanks. One quick note: could you please add cli-testlab-based test for this?

@yeonhoyoon
Copy link
Contributor Author

OK, but I'm not familiar with cli-testlab, so I will need some time to look into it.

@kibertoad
Copy link
Collaborator

@yeonhoyoon Here is a super simple example: https://github.com/knex/knex/blob/master/test/cli/version.spec.js

and you can check other files in same directory if you'd need more than that

@yeonhoyoon
Copy link
Contributor Author

@kibertoad
Thanks for your feedback, I added some specs to test help command.
There wasn't an assertion helper in the cli-testlab for expecting the output to not include a given string, so I used the raw stdout variable instead to test the desired behavior.

knex cli should output help only when there are no arguments.
current implementation always outputs help, even with correct arguments, such as `knex migrate:status`
the argument length should be checked via process.argv.
reference: https://github.com/tj/commander.js/#outputhelpcb

Add tests for help command
@kibertoad
Copy link
Collaborator

Much appreciated!

There are linting errors in the test:

/home/travis/build/knex/knex/test/cli/help.spec.js
   6:7  error  'cliPkg' is assigned a value but never used  no-unused-vars
  30:7  error  'expect' is not defined                      no-undef

After those are fixed, this can be merged :)

@kibertoad
Copy link
Collaborator

@yeonhoyoon BTW, thank you for mentioning missing functionality in cli-testlab. I've added notExpectedOutput parameter and published 1.9.0, but it's a small thing, feel free to update it or leave as it is.

@yeonhoyoon
Copy link
Contributor Author

@kibertoad
Apologies for leaving unused variables in the code, and thanks for quickly adding features to cli-testlab. I've changed the tests to use the newly added functionality.
Please let me know if you have any other feedback.

@kibertoad kibertoad merged commit 4a2fa3b into knex:master Jan 14, 2020
@kibertoad
Copy link
Collaborator

Thanks! <3

@kibertoad
Copy link
Collaborator

Released in 0.20.8

@yeonhoyoon yeonhoyoon deleted the patch-1 branch January 15, 2020 01:32
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.

None yet

3 participants