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

Establish error message approach #6

Closed
joesepi opened this issue Apr 30, 2021 · 8 comments · Fixed by #40
Closed

Establish error message approach #6

joesepi opened this issue Apr 30, 2021 · 8 comments · Fixed by #40
Assignees
Labels
question Further information is requested

Comments

@joesepi
Copy link
Collaborator

joesepi commented Apr 30, 2021

See: #1 (comment)

Whoops!

@joesepi
Copy link
Collaborator Author

joesepi commented May 14, 2021

Let's try to mirror what Node does in a minimal fashion but could be easily ported over when/if the time comes.

@joesepi
Copy link
Collaborator Author

joesepi commented May 14, 2021

Another question in today's meeting: Should we ever Throw an error? Or should we always return and on error, return an error object?

The consensus is we should avoid throwing an error and prefer to return an error object and allow the consumer of parseargs to manage error handling.

@ljharb
Copy link
Member

ljharb commented May 14, 2021

Typically I throw a string from argument validation, because throwing an error causes node to print stack trace info that's undesirable.

It would also be fine to let the consumer handle it, but since there's no robust way of identifying error objects, i'd prefer a two-channel mechanism like Promise.

@joesepi
Copy link
Collaborator Author

joesepi commented Oct 1, 2021

@darcyclarke darcyclarke added the question Further information is requested label Oct 27, 2021
@shadowspawn
Copy link
Collaborator

shadowspawn commented Nov 14, 2021

For my command-line work, I make a distinction between errors which are due to programmer error and errors which are encountered at runtime due to usage errors on the command line. For the programmer errors I throw Error, very noisy but the stack is at least potentially helpful for the programmer to find and fix the coding error.

@ljharb
Copy link
Member

ljharb commented Nov 14, 2021

In my yargs packages i throw strings instead of errors for arg validation, for that reason.

@bcoe
Copy link
Collaborator

bcoe commented Dec 4, 2021

I think it's less important that we figure out the wording of errors, when we merge into the Node.js codebase we will model after other errors...

More importantly, I like @shadowspawn's suggestion that we go through the codebase and determine which errors could be categorized as input errors, vs., programming errors, and decide how we handle each:

  • do we throw on input errors, or perhaps populate an error string instead?

@bcoe bcoe added this to the Merge into Node.js milestone Dec 4, 2021
@shadowspawn
Copy link
Collaborator

shadowspawn commented Dec 4, 2021

do we throw on input errors, or perhaps populate an error string instead?

There have been a few people suggesting that parse never "fails" on user input, and perhaps error information could be a property in the results. I see the attraction and interested in trying that approach. [Edit: I am no longer interested in quietly returning errors, doubtful it will be useful enough.]

In line with the description and intent of strict, the implementation could be parseArgs throws the input-error string if strict, and quietly returns the input-error string if !strict.

(I am personally of the view that if the input does not match the programmers intentions then the outcome is in doubt. But a key part of the proposal is zero-setup parsing, and I can turn on strict to get more certainty.)

bcoe added a commit that referenced this issue Jan 22, 2022
Move to Node.js style errors and validation to make it easier to
drop into Node codebase.

Fixes #6
@bcoe bcoe closed this as completed in #40 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants