Refactoring the option.Parse method #930
Conversation
Co-authored-by: Hao Chen <[email protected]>
pkg/option/option.go
Outdated
| if opt.ShowVersion { | ||
| return version.Short, nil | ||
| } | ||
|
|
||
| if opt.ShowHelp { | ||
| return opt.flags.FlagUsages(), nil |
There was a problem hiding this comment.
Removing these lines changes the logic, that it may fail to show the version or help information if the rest code returns an error.
There was a problem hiding this comment.
hi @localvar, I think it's okay for the program to return an error if the user includes some incorrect commands when starting the service while checking the version or help information.
There was a problem hiding this comment.
we hope the show version and usage are simple so that they never fail, take show usage as an example: if the user doesn't know the usage, then it would be hard for him/her to understand what was wrong in the command line.
There was a problem hiding this comment.
I still think it's okay. Consider the following examples,
# Same behavior
easegress-server --help
easegress-server some-not-exists-command --help
# Different behaviors
// Previously, help information was displayed, but now an error is returned instead.
easegress-server -f not_exists_file.yaml --help
where only an incorrectly specified configuration file could cause different behavior between the two.
If the user specifies an incorrect configuration file (or if the file content is incorrect), returning an error directly without showing help may is acceptable.
There was a problem hiding this comment.
No, if the user wants the version and usage, we need always show the information, we all know there are jokes that a user asks where the any key is, so we need to keep the help information as easier to get as possible.
and from the current code, not only an incorrect configuration file will return an error.
and from the maintenance view, the code will change from time to time, even if only an incorrect configuration file returns an error now, developers may add other error cases later, and it is very possible for them to forget to update these codes at the same time, that'd result in a bad user experience.
There was a problem hiding this comment.
Do you have any good suggestions, or should we just close it?
There was a problem hiding this comment.
if opt.ShowVersion || opt.ShowHelp {
return nil
}| @@ -159,7 +158,7 @@ func New() *Options { | |||
|
|
|||
| opt.flags.IntVar(&opt.StatusUpdateMaxBatchSize, "status-update-max-batch-size", 20, "Number of object statuses to update at maximum in one transaction.") | |||
|
|
|||
| opt.viper.BindPFlags(opt.flags) | |||
| _ = opt.viper.BindPFlags(opt.flags) | |||
There was a problem hiding this comment.
why make this change and the changes at L183, L187?
There was a problem hiding this comment.
this is a minor update because the IDE will display a warning if we don't handle the error.
There was a problem hiding this comment.
that's ok, but may I know which IDE you are using? On my side, vs code never reported this warning, and this could not be a help as the error is still not handled.
There was a problem hiding this comment.
I'm using IntelliJ IDEA, and here's an example where a warning is displayed in IntelliJ IDEA:
godruoyi
force-pushed
the
optimal_configuration_parse
branch
3 times, most recently
from
566cbaa
to
da3825d
Compare
godruoyi
force-pushed
the
optimal_configuration_parse
branch
from
da3825d
to
db9de99
Compare
The
option.Parsemethod should return only failure or success without other logic.