feat: customize error formatting and styling

[elliot-nelson]

SUMMARY

• Customize unexpected error formatting with .unexpectedErrorFormatter().
• Customize unexpected error styling with unexpectedError style hook.

DETAILS

require('sywac')
  .command('start', /* ... */)
  .command('stop', /* ... */)
  .unexpectedErrorFormatter(error => error.message)
  .style({ unexpectedError: str => chalk.red(str) })
  .parseAndExit()

This PR gives two ways to customize the formatting and/or the styling of the error output of an unexpected error (an error thrown by a run() or check() handler, or added manually using context.unexpectedError).

[nexdrew]

Awesome, thanks for doing this! I appreciate that you were able to keep the implementation simple and straightforward.

I just have a handful of comments/questions. Let me know what you think. Thanks!

[nexdrew]

Maybe we could move this to the configure method, so there's an alternate way of setting/overriding it without calling .unexpectedErrorFormatter(fn).

Perhaps something like this:

this._errorFormatter = opts.errorFormatter || this._errorFormatter // expects a function

[nexdrew]

I kinda wish the name of the variable more closely resembled the name of the Api method, but I also prefer shorter names to longer ones.

I don't know, this is probably fine. (No action needed.)

[nexdrew]

It seems a little odd/unnecessary/inefficient to me to pass all helpOpts to the Context, but I guess it doesn't hurt anything. (No action needed.)

[elliot-nelson]

I feel like, once you take the plunge to say "we sometimes require styling in Api", you should just pass it all.

I'd be totally comfortable including it nested if you felt that was better encapsulated -- i.e. instead of ...helpOpts pass in helpOpts as a key.

[nexdrew]

I'd be totally comfortable including it nested if you felt that was better encapsulated -- i.e. instead of ...helpOpts pass in helpOpts as a key.

My gut instinct tells me this would be better than how it currently is, though I can't objectively defend that.

If you're willing to make that change, I'd appreciate it.

[nexdrew]

I wish there was a way to do this where we didn't have to split style hook executions between different locations, but I can't think of a better way of doing it than this.

The only potential problem I see is that this doesn't leverage the all style hook (as a final wrapper for all other styled strings), but it wasn't doing that before, and I'm not sure if a CLI author would expect the all hook to apply to a formatted error message or not (currently it only applies to help text and expected CLI messages). What do you think?

[elliot-nelson]

I guess it depends what you're doing with all. Possible use case:

all: text => {
    // Example weird use case: always return output as a JSON blob containing lines as an array,
    // for consumption by some internal API.
    return JSON.stringify({ lines: text.split('\n') })
}

If this is how I was using sywac, then I would want any output error messages to be run through it as well (although, in this case, I'm probably using parse() and could just examine .errors instead of the output).

My concern about just wrapping it with all is that you would potentially run afoul of all handers written just to handle the expected help text screen and not a generic error. I almost feel you need to pass a second boolean option to tell the function whether this is a "help text" context or "unexpected error" context... but if you did that, and the user actually uses the boolean to make a decision, then I feel you could just as easily require them to put the logic in the unexpected error handler:

all: text => {
    return JSON.stringify({ lines: text.split('\n') })
},
unexpectedError: text => {
    return JSON.stringify({ lines: text.split('\n') })
},

TLDR: This is a good point but I think I land on the side of "all does not apply to unexpected errors, because that's more flexible than the alternative".

[nexdrew]

I think I agree with you. Thanks for the feedback!

[nexdrew]

Might be good to also have a test that applies formatting/styling to an error thrown from a (nested) command.

[nexdrew]

On another note, when I first saw this, I thought the _errorFormatter property should be propagated down to child Api instances via newChild, but then I realized that it's only applied at the top-level since that's where the single Context instance (per parse) is initialized.

When documenting this, we should note that both .unexpectedErrorFormatter(fn) and the unexpectedError style hook can only be applied at the top-level (i.e. cannot be applied/overridden by command configuration), because a single "top-level" Context instance is used for all (potentially nested) command levels.

[nexdrew]

In preparation for implementing #65, perhaps we could go ahead and throw an error from .unexpectedErrorFormatter(fn) if it's called on a child Api instance?

Just trying to think in terms of setting a precedent for the future. Otherwise, I'm completely fine if the calls simply get ignored in the initial implementation of this feature.

[elliot-nelson]

My preference is to start with no error for now -- in my opinion, if people complained about being bitten by this bug, then the "right fix" isn't to prevent people from passing the error formatter to subcommands, it's to fix it so it works as the user was assuming.

(That's out of scope for this PR, but I think it's possible if we cared enough -- we'd need to capture an unexpected error thrown at the Command level and capture the error formatter from that context, and then ensure that one got used if and when an unexpected error was printed. I consider this a "potential future todo" that's better than just preventing people from using the option.)

[elliot-nelson]

New commit pushed:

• Changed all references to the new formatter to unexpectedErrorFormatter (although it's longer, I think it's less confusing to refer to a value by the same name everywhere we can).
• Pass helpOpts as a property instead of collapsing into the rest of the options for Context.
• Added a test where an error thrown by a subcommand is styled & formatted.