Handle error messages for unknown args better

[secondary-smiles]

[what-the-diff[bot]]

• Added a try/catch block to the app.ts file
• Changed some of the error messages in useFlags() and added an internalSetVerbosity function that is called when there's an error with flags
• Made changes to TeaError class, including adding new cases for not-found: flag errors (which are thrown by useArgs()) and changing how panic works so it throws a normal Error instead of a TeaError object

[lino-levan]

I'm going to be honest, I'm confused as to how this helps.

[secondary-smiles]

Previously, if tea was passed a flag that it didn't recognize it would throw an error which wasn't consistent with most of the other tea errors. Also, if it were passed '+', '-', or '--' without any flags or commands it would try to continue which can lead to unrelated errors. This PR just adds a little bit more checking while parsing flags as well as a better panic handler that produces a cleaner output.

[lino-levan]

That makes sense. I guess my question more specifically is:

why is

 if (message) {
   console.error(`panic: ${message}`)
 }
 Deno.exit(1)

better than

throw new Error(message)

I'm of the camp that believes we should use Deno.exit sparingly. Is throwing an error not good enough? Are we using this to get around try {} catch {} blocks? I'm curious to hear the reasoning here.

[secondary-smiles]

When Deno throws an error it leaves a trace.

error: Uncaught Error: unknown flag: -g
                        throw new Error(`unknown flag: -${c}`);
                              ^
    at useArgs (file:///opt/tea.xyz/src/v0.16.0/src/hooks/useFlags.ts:183:31)
    at file:///opt/tea.xyz/src/v0.16.0/src/app.ts:12:27

While I agree that Deno.exit is a last resort method, I think it's worth using to get rid of the trace in production. I don't know if Deno provides an option to hook Error throws but if there is then that would probably be a better thing to do. I couldn't find anything like that in the docs though.

[jhheider]

While I agree that Deno.exit is a last resort method, I think it's worth using to get rid of the trace in production. I don't know if Deno provides an option to hook Error throws but if there is then that would probably be a better thing to do. I couldn't find anything like that in the docs though.

Perhaps the middle ground is to handle this based on Verbosity. We could throw with the stack trace at a higher Verbosity, and exit more cleanly with just a message and an exit code at lower levels.

It might also be useful to provide the tea-specific error code to exit, for more useful constructs.

[lino-levan]

Agree with @jhheider on this one. Verbosity should play a role in this 100%.

[secondary-smiles]

I also agree. I'll revert the panic() usage and function and see about making a new PR to address it.

[jhheider]

I think you could probably just throw in a

const { verbosity } = useFlags()

switch (verbosity) {

and customize behavior from there.

[lino-levan]

I think @secondary-smiles could probably just do this in this PR. @jhheider's example implementation of this LGTM.

[secondary-smiles]

Sure thing, I'll get to it in a bit.

[mxcl]

This is a welcome addition.

We create TeaError for errors that are known and acceptable and should not have a stack trace. I believe panic() should continue to show a trace in all situations since we use it to mean “if this happens it is programmer error”.

So create a new TeaError category?

[secondary-smiles]

Sure. I haven't gotten super familiar with the sourcecode yet, but I'll take a look later and work on a TeaError for arg parsing.

[mxcl]

Thanks for the contribution 👏🏻