Add new type to represent validating required flag error #2274
Conversation
Signed-off-by: sivchari <[email protected]>
|
|
| // RequiredFlagError represents a failure to validate required flags. | ||
| type RequiredFlagError struct { | ||
| Err error | ||
| } | ||
|
|
||
| // Error satisfies the error interface. | ||
| func (r *RequiredFlagError) Error() string { | ||
| return r.Err.Error() | ||
| } | ||
|
|
||
| // Is satisfies the Is error interface. | ||
| func (r *RequiredFlagError) Is(target error) bool { | ||
| err, ok := target.(*RequiredFlagError) | ||
| if !ok { | ||
| return false | ||
| } | ||
| return r.Err == err | ||
| } | ||
|
|
||
| // Unwrap satisfies Unwrap error interface. | ||
| func (r *RequiredFlagError) Unwrap() error { | ||
| return r.Err | ||
| } |
There was a problem hiding this comment.
This seems to be overkill.
I'm doubtful about the implementation.
I'm not sure you need a structure, especially while the type is a simple wrapper here.
If you had stored the fields name, I would have understood, but here you don't.
Also I will have double check about the need of Is method. Usually, adding Unwrap is enough.
I will come back to you later with feedbacks and maybe another implementation
|
This feature was also implemented by PR #2266, and that PR also includes an error type added by an older PR, #2178 (CC @mvo5). Seeing as how we have three different PRs aiming to achieve the same goal of making it possible to determine the type of and unwrap errors generated by Cobra, I think it would be a good idea to focus our combined efforts around a single implementation. I might be a bit biased as the author of #2266, but that one covers pretty much every user-caused error type that Cobra can return. For what it's worth mentioning as well, a similar effort was made to add error structs to spf13/pflag. It would be great to have |
|
Thanks for cross referencing things. I saw the previous one @eth-p Then I saw this one, I thought it was from the same author, that had closed then reopened a new PR I have already shared my concerns with the implementation in this PR |
Currently, when we met the ValidateRequiredField error, we should compare as a raw string.
This proposal aims to compare by using errors.Is/As and csting from interface to the type.