-
Notifications
You must be signed in to change notification settings - Fork 303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove unwrapping in error Is
methods
#2269
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
package plugin | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"sort" | ||
"strings" | ||
|
@@ -19,14 +18,14 @@ func (e *DeprecatedNameErrors) IsEmpty() bool { | |
return e == nil || len(e.errs) == 0 | ||
} | ||
|
||
// Errors returns the contained set of errors in sorted order | ||
func (e *DeprecatedNameErrors) Errors() []DeprecatedNameError { | ||
// Unwrap returns the a slice of errors in a stable order | ||
func (e *DeprecatedNameErrors) Unwrap() []error { | ||
if e == nil { | ||
return nil | ||
} | ||
|
||
if e.errs == nil { | ||
return []DeprecatedNameError{} | ||
if len(e.errs) == 0 { | ||
return []error{} | ||
} | ||
|
||
errs := make([]DeprecatedNameError, 0, len(e.errs)) | ||
|
@@ -41,13 +40,18 @@ func (e *DeprecatedNameErrors) Errors() []DeprecatedNameError { | |
return errs[i].old < errs[j].old | ||
}) | ||
|
||
return errs | ||
out := make([]error, 0, len(errs)) | ||
for i := range errs { | ||
out = append(out, &errs[i]) | ||
} | ||
Comment on lines
+43
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the alternative is to make Hang on, does anything use the order of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's nice for the user. I wrote tests for it, so they will break There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. Maybe I didn't 🤷♂️. Still think it's nice for the user. |
||
|
||
return out | ||
} | ||
|
||
// Error returns each contained error on a new line | ||
func (e *DeprecatedNameErrors) Error() string { | ||
builder := strings.Builder{} | ||
for i, err := range e.Errors() { | ||
for i, err := range e.Unwrap() { | ||
_, _ = builder.WriteString(err.Error()) | ||
if i < len(e.errs)-1 { | ||
_, _ = builder.WriteRune('\n') | ||
|
@@ -73,15 +77,11 @@ func (e *DeprecatedNameErrors) Append(errs ...DeprecatedNameError) *DeprecatedNa | |
return e | ||
} | ||
|
||
// Is returns true if and only if a error that is wrapped in target | ||
// contains the same set of DeprecatedNameError as the receiver. | ||
// Is returns true if and only if a target contains the same set of | ||
// DeprecatedNameError as the receiver. | ||
func (e *DeprecatedNameErrors) Is(target error) bool { | ||
if e == nil { | ||
return target == nil | ||
} | ||
|
||
var targetErr *DeprecatedNameErrors | ||
if !errors.As(target, &targetErr) { | ||
targetErr, ok := target.(*DeprecatedNameErrors) | ||
if !ok { | ||
return false | ||
} | ||
|
||
|
@@ -110,18 +110,10 @@ func NewDeprecatedNameError(oldName, newName string) DeprecatedNameError { | |
} | ||
|
||
func (e *DeprecatedNameError) Error() string { | ||
return fmt.Sprintf(" deprecated: %q\nreplacement: %q\n", e.old, e.new) | ||
return fmt.Sprintf("deprecated: %q\nreplacement: %q\n", e.old, e.new) | ||
} | ||
|
||
func (e *DeprecatedNameError) Is(target error) bool { | ||
if e == nil { | ||
return target == nil | ||
} | ||
|
||
var targetErr *DeprecatedNameError | ||
if !errors.As(target, &targetErr) { | ||
return false | ||
} | ||
|
||
return e.old == targetErr.old && e.new == targetErr.new | ||
terr, ok := target.(*DeprecatedNameError) | ||
return ok && *e == *terr | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're here, you could update
Errors
to beUnwrap
that returns[]error
. I don't think any call sites ofErrors
use anything other than theError
method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done