Show help text on demand

[domn1995]

Notable changes:

• Created a CommandErrorDisplayOptions flags enum for customizing error output between showing no output, showing the exception message, or showing the help text.
• Created unit tests addressing the above.
• Updated readme with example usage.

Note:

• I did not update the version information. Wasn't too sure how you wanted to handle that.

Closes #48

[Tyrrrz]

Thanks! I'll take a look at it when I have more time.

[Tyrrrz]

Hi, I finally found some time to look at it. I see you have introduced an enum to configure whether the error/help is printed or not.

I think we can simplify it and just have the boolean flag for whether the help text should be printed. Then, if the exception contains a message, show it as well. Otherwise, print stack trace only if help text is not shown.

This makes things simpler and seems to be reasonable in terms of user expectations.

[domn1995]

Yeah, I was weighing the pros and cons of enum vs bool and settled on enum for the following reasons:

• There's a somewhat general consensus that enums are better than bools because they are more extensible and expressive. Think string.Split("foo", StringSplitOptions.RemoveEmptyEntries) vs string.Split("foo", true).
• Allows users to optionally display an error message before the help text. This seems like a very useful feature to help clarify to the user why we're showing the help text and many command line tools work this way. Using git as an example:

  
  > git clone
  You must specify a repository to clone.

usage: git clone [] [--] [

]

-v, --verbose         be more verbose
-q, --quiet           be more quiet
--progress            force progress reporting
...

[Tyrrrz]

With the approach I suggested, we'd be able to show the error if it was provided. The error logic should remain the same, but if help text is also shown, then there's no reason to print the stack trace. Stack trace should be the last resort if there's no message and help text is not shown.

As for enum vs bool, you are right. However, I want to deliberately limit the number of permutations here to make it easier to reason about the framework and test it. I would prefer to have a good default behavior that covers 90% of use cases than 3 alternative workflows that cover 100% :)

[domn1995]

Yeah the extra complexity is definitely a minus.

So with the bool solution, if the user doesn't want to display an error message before the help text, they would do throw new CommandException(showHelp: true)
whereas if they wanted an error message before the help text they would do throw new CommandException(message: "Error message", showHelp: true).

Does that jive with you? If so, I'll update the PR.

[Tyrrrz]

Yes, basically:

• throw new CommandException(message: "Error message", showHelp: false)

Prints just the error message, as it does now.

• throw new CommandException(message: "Error message", showHelp: true)

Prints the error message followed by help text.

• throw new CommandException(message: null, showHelp: true)

Prints just the help text, without error message.

• throw new CommandException(message: null, showHelp: false)

Prints the stack trace, as it does now.

[domn1995]

Sounds good. I'll update the PR.

[domn1995]

Found an issue with the existing codebase as I was implementing this:

It looks like we'll never get a stack trace if we pass null into an Exception's constructor's message parameter. That's because the .NET Exception.cs code for the Exception.Message property checks if it is null and returns a default value if it is. See the code below:

https://github.com/microsoft/referencesource/blob/a7bd3242bd7732dec4aebb21fbc0f6de61c2545e/mscorlib/system/exception.cs#L136

I suggest we make CommandException and CliFxException's constructor's message parameter non-nullable to prevent future bugs related to this behavior.

Additionally, the code in CliApplication.cs will never hit the null case because of the aforementioned behavior. The consequence of all of this is that there's no clean way to implement your last two use cases from the reply above because throw new CommandException(message: null, ...) will always print Exception of type 'CliFx.Exceptions.CommandException' was thrown. with the current logic.

The only way to get a stack trace currently is to throw new CommandException(message: "", ...).

I don't see any specs testing the stack trace printing behavior you described, so I suggest creating an issue to implement one or more such tests.

Now back to the original feature request, the enum solution is looking more attractive again, as it will allow us to implement showing just the help text, the error message and help text, or the stack trace based on an ErrorDisplayOptions enum and seems like the cleanest way to get original behavior that was intended.

How would you like to proceed? Also let me know if you want some more data.

Cheers!

[Tyrrrz]

Ha, i did not know this.

There actually exists a spec for that, but it doesn't verify that what's printed is stack trace, just that something is printed in the event that no message was provided.

https://github.com/Tyrrrz/CliFx/blob/1dab27de551c8fcb5abd2656b5372c0091a77eb5/CliFx.Tests/ErrorReportingSpecs.cs#L60-L82

Regarding enum, I guess my main concern is that that API lets me throw an exception with an error message, but also configure it to not show that error message. In that case it's not clear what's the error message for. With the aforementioned approach it's more invariant: you provide a message -> it gets shown, otherwise it doesn't. Of course, .NET's behavior makes it a bit more annoying to do, but it's possible to extend the constructor so that it implicitly sets something like IsErrorMessageProvided which is evaluated as !string.IsNullOrWhiteSpace(message).

[domn1995]

Yeah, in hindsight I probably shouldn't have made a flag for showing no error message. :)

Anyway, I'll implement the bool solution with the null or empty handling logic inside the exception. I'll also make sure we have a unit test for the stack trace case and update the PR.

[Tyrrrz]

Cool, let me know when I should look at it again.

[domn1995]

Ready for you to take a look at, @Tyrrrz!

Notable changes:

• Added a BaseCliFxException to remove code duplication between CommandException and CliFxException related to new help text logic. This also lets us optionally show help text when CliFxExceptions are thrown.
• Updated CliFxApplication with help text and error message handling logic.
• Created unit tests addressing the new help text features.
• Updated the existing stack trace unit test that wasn't behaving correctly.
• Updated the readme with details on customizing error reporting to show help text, or error message then help text.
• Updated CliFx.Dummy project with examples of the aforementioned functionality.

Closes #49 and facilitates implementation of #16

[domn1995]

@Tyrrrz, all requested changes were made. Hopefully ready for a merge :)