Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upmake missing required flags an ExitCoder so that app exit code handling treats this as an error #1163
Conversation
extend app.Run() such that missing flags that are set as Required will be handled by OSExiter defines a fixed error status for missing default flags extends the requiredFlagsErr interface to satisfy ExitCoder() implement ExitCode() on errRequiredFlags struct to return the fixed error status then when App is checking requiredFlags, call handleExitCoder if there is an error returned here
|
LGTM :) |
|
v1 is in maintenance mode and this is a very very small new feature, and a potentially breaking one at that. So I'm not sure it's good idea for us to merge this. I could be convinced otherwise, though! |
Thank you for the review! I can understand this point of view. I don't think this changes is much of a breaking one though, specific to this case, I do find it difficult to imagine who is relying on the specific implementation of required flags, particularly in expecting a successful exit code for an failure to run, but this is the nature of changing anything established, there's no way to predict every user's expectations, I grant. I am quite happy for it to be either merged or closed, and once again, thanks for your time and efforts. |
cmstrickland commentedJul 22, 2020
What type of PR is this?
What this PR does / why we need it:
modifies errRequiredFlags so that it can be used as an ExitCoder, and then changes the error handling in app.Run so that it does not bypass the osExit handling
Special notes for your reviewer:
I just picked a value to use for the exit status, it is hard coded
This patch is only against v1, I am not currently using v2, unsure if it is applicable there.
Testing
I added a simple test to app_test that the handler is called when a required flag is not supplied
Release Notes