-
Notifications
You must be signed in to change notification settings - Fork 37
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
Introduce Go 1.13 compatible error types #42
Conversation
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.
A number of questions about errors.As()
vs errors.Is()
. Otherwise LGTM 🚀
session_test.go
Outdated
@@ -78,7 +78,8 @@ func TestGetSession(t *testing.T) { | |||
Config: &Config{}, | |||
Description: "no configuration or credentials", | |||
ExpectedError: func(err error) bool { | |||
return errors.Is(err, ErrNoValidCredentialSources) | |||
var credentialError NoValidCredentialSourcesError | |||
return errors.As(err, &credentialError) |
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.
Should this be errors.Is()
to just return the boolean? errors.As()
populates a variable that we're throwing away immediately
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.
You would think that (I know I certainly do!), but using errors.Is()
is for error value checking, so you get this compiler error when trying to check against the type itself, e.g. errors.Is(err, NoValidCredentialSourcesError)
:
type NoValidCredentialSourcesError is not an expression
Quick example: https://play.golang.org/p/3UcimRc0wJv
Maybe I'll drop in a quick func IsNoValidCredentialSourceError(err error) bool
since this could potentially be useful to check downstream.
@@ -1058,23 +1059,23 @@ func TestGetSessionWithAccountIDAndPartition(t *testing.T) { | |||
config *Config | |||
expectedAcctID string | |||
expectedPartition string | |||
expectedError string | |||
expectedError bool |
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.
👍
awsauth_test.go
Outdated
@@ -434,7 +435,8 @@ func TestAWSGetCredentials_shouldErrorWhenBlank(t *testing.T) { | |||
cfg := Config{} | |||
_, err := GetCredentials(&cfg) | |||
|
|||
if err != ErrNoValidCredentialSources { | |||
var credentialError NoValidCredentialSourcesError | |||
if !errors.As(err, &credentialError) { |
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.
Should this be errors.Is()
since we're just checking the type?
if sess == nil { | ||
t.Error("GetSession(c) resulted in a nil session") | ||
if !tc.expectedError { | ||
t.Fatalf("expected no error, got: %s", err) |
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.
I like the more concise errors here
Reference: #24 Callers can configure their name and documentation URL to properly customize the error messaging.
5d1fea5
to
26deb65
Compare
Community Note
Closes #24
Callers can configure their name and documentation URL to properly customize the error messaging.