Tyrrrz / CliFx

Class-first framework for building command-line interfaces
MIT License
1.48k stars 60 forks source link

Print help text on specific domain exceptions #51

Closed domn1995 closed 4 years ago

domn1995 commented 4 years ago

Closes #16.

Notable Changes

@Tyrrrz, can you double check that you're happy with these changes? If you like it, how would you prefer I implement unit tests? Should I augment the existing ArgumentBindingSpecs that throw the end-user-facing exceptions to also check that we're printing the help text or make brand new ones that assert we're throwing a CliFxException AND displaying the help text?

Tyrrrz commented 4 years ago

I'm not a big fan of virtual properties. I think it's better to have it as an auto-property and set it through the base constructor (it can default to HResult in one of the overloads).

Regarding unit tests: we don't write any :) As for functional tests, I'd say add a test to ErrorReportingSpecs any of the simpler user-facing errors to make sure it also contains help text.

domn1995 commented 4 years ago

@Tyrrrz, ready for your input again. I've gone ahead and moved the exit code logic to BaseCliFxException and added an FT for showing the help text on invalid user input.

Tyrrrz commented 4 years ago

That looks good. Now I'm wondering, should we just merge BaseCliFxException and CliFxException (and have CommandException inherit from CliFxException)? What do you think? I don't know if there's much benefit in keeping them separate.

domn1995 commented 4 years ago

Yeah, I think that's a good idea. Want me to update the PR?

Tyrrrz commented 4 years ago

Sure, go ahead.

domn1995 commented 4 years ago

@Tyrrrz, done and hopefully ready for a merge :)

Tyrrrz commented 4 years ago

Thanks!