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

Errors are printed twice if exitProcess is false #270

Closed
tomyam1 opened this issue Sep 30, 2015 · 8 comments
Closed

Errors are printed twice if exitProcess is false #270

tomyam1 opened this issue Sep 30, 2015 · 8 comments

Comments

@tomyam1
Copy link

tomyam1 commented Sep 30, 2015

When exitProcess is false, errors are outputted twice.
See failing spec in: #269

An error is first outputted here.
It's then thrown a few lines below.

It will then be caught and outputted again here.

I think a simple solution would be to create something like a ParseError type. When we catch an error of this type up the stack - we don't print it again.

Will you accept a PR?

@nexdrew
Copy link
Member

nexdrew commented Sep 30, 2015

@tepez Thanks! Looks like .exitProcess(false) is not very popular and could use some love. :)

Just to be clear, the help content (if showHelpOnFail is true) and error message are output via usage.fail(), which is called twice (if getExitProcess() returns falsy) - first in a validation method (like customChecks() or requiredArguments()) and second in the catch block of the argv getter method.

I think your proposed solution is a good one, but I'm going to investigate other options for sake of due diligence. I'll report back with any findings/ideas.

@tomyam1
Copy link
Author

tomyam1 commented Sep 30, 2015

Thanks @nexdrew. I'll wait.

There is another issue with exitProcess(false) that we could easily solve with a new ParseError type. Currently, when we catch an error that was thrown somewhere in the CLI main function, we have no way to know if it was thrown by yargs or somewhere else.

If it was thrown by yargs - it was already printed so there is no need to print it again.
If it was thrown somewhere else - we do need to print it.

@nexdrew
Copy link
Member

nexdrew commented Sep 30, 2015

I do agree we should fix this problem, but note that a possible workaround would be to use your own failure handler via .fail(fn), which will bypass failure message output and error throwing.

@tomyam1
Copy link
Author

tomyam1 commented Sep 30, 2015

Great idea @nexdrew !
This simple function solved all duplications for me:

yargs.fail((msg) ->
  throw new Error(msg)
)

@nexdrew
Copy link
Member

nexdrew commented Sep 30, 2015

@tepez Great, glad I could help!

As far as potential solutions to this problem goes, here's what I have so far:

  1. Add private variable to lib/usage.js and use it within the fail() method to only output help content and failure message once. (This is rather trivial to implement and does not affect any current unit tests, but I'm not sure if it has other side effects.)
  2. Add a public API method similar to .showHelpOnFail() to disable showing failure messages on parsing/validation errors.
  3. Throw custom error types, as proposed. Not sure if it would be better to use multiple types based on where (or at which point) the error is thrown (e.g. ParseError for parsing, ValidationError for validation, etc) or use a single type for all yargs-generated errors (e.g. YargsError).
  4. Some combination of the above.

Thoughts?

@nexdrew
Copy link
Member

nexdrew commented Oct 1, 2015

For the first potential solution mentioned above, see PR #273.

@tomyam1
Copy link
Author

tomyam1 commented Oct 1, 2015

LGTM

@nexdrew
Copy link
Member

nexdrew commented Oct 4, 2015

Closing this since PRs #269 and #273 have been merged.

@nexdrew nexdrew closed this as completed Oct 4, 2015
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

No branches or pull requests

2 participants