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 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

This comment has been minimized.

Copy link
Contributor

timorthi commented Jan 8, 2020

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 8, 2020

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

@yeonhoyoon

This comment has been minimized.

Copy link
Contributor Author

yeonhoyoon commented Jan 10, 2020

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 10, 2020

@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

This comment has been minimized.

Copy link
Contributor Author

yeonhoyoon commented Jan 13, 2020

@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
@yeonhoyoon yeonhoyoon force-pushed the yeonhoyoon:patch-1 branch from 5b807d1 to 606f944 Jan 13, 2020
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 13, 2020

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 13, 2020

@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

This comment has been minimized.

Copy link
Contributor Author

yeonhoyoon commented Jan 14, 2020

@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.

@yeonhoyoon yeonhoyoon force-pushed the yeonhoyoon:patch-1 branch from dbe7018 to 85a78ea Jan 14, 2020
@kibertoad kibertoad merged commit 4a2fa3b into knex:master Jan 14, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 14, 2020

Thanks! <3

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Jan 14, 2020

Released in 0.20.8

@yeonhoyoon yeonhoyoon deleted the yeonhoyoon:patch-1 branch Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.