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

Keep command out of program.args and add unknown to .action params #1048

Merged
merged 1 commit into from Sep 21, 2019

Conversation

shadowspawn
Copy link
Collaborator

This reimplements #1030 after the move to Jest. See #1030 for description and discussion.

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

Thank you.

@shadowspawn shadowspawn merged commit cc4a525 into tj:develop Sep 21, 2019
@shadowspawn shadowspawn deleted the feaure/386-args-2 branch September 21, 2019 06:21
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Sep 21, 2019
@xPaw
Copy link

xPaw commented Nov 1, 2019

This PR now makes it impossible to detect whether any command has run, like the example in: #7 (comment)

We want to print help when no command has run, and it doesn't really seem to be possible (process just exists without printing anything). Checking process.argv.length is not really a good option either.

And on top of that command:* is not triggered either if you pass no command.

@shadowspawn
Copy link
Collaborator Author

@xPaw
Sorry you hit issues.

This PR does change the behaviour in an area that people have written code based on undocumented details of the previous behaviour, so will cause some breakages. Hopefully we can work out a more robust way to achieve the same behaviours, and where appropriate add migration steps to the Release Notes.

I test process.argv.length in my own CLI program for the specific case that user ran my command with no arguments:

// Show help if no command specified.
if (process.argv.length === 2) {
  program.help();
}

Please open a new issue with an example of what you are trying to do and some example code.

@xPaw
Copy link

xPaw commented Nov 1, 2019

process.argv.length is not really an elegant solution.

Issue was already made in 2011 about this: #7, you could just re-open that.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Nov 1, 2019

#7 was closed, with some up-voted example code supplied which tests program.args rather than process.argv: #7 (comment)

Does that work for your case?

@xPaw
Copy link

xPaw commented Nov 1, 2019

This PR specifically "broke" testing program.args as it not longer pushes the found command to it. That's what I did before.

My current "fix" was to use program.rawArgs

thelounge/thelounge@e09599a#diff-25f700bb76bbf957569bd288fbf7d328L63-R63

@shadowspawn
Copy link
Collaborator Author

Ok, I see the problem. Not sure about best solution yet, but that is reasonable.

@shadowspawn
Copy link
Collaborator Author

I have added a migration tip to the Release Notes, but it might be an issue that needs a better solution. The detection of unrecognised commands and missing commands when using action handlers is a bit fragile.

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