Skip to content
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.

Sign up for GitHub

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

Stop initialising the IsUnexpected flag to false for new errors #54

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Stop initialising the IsUnexpected flag to false for new errors #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45a87db
Stop initialising the IsUnexpected flag to false for new errors
mattcottam-monzo Aug 30, 2024
3c71ada
Update comment to give extra guidance
mattcottam-monzo Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,8 @@ func (p *Error) Retryable() bool {
}

// Unexpected states whether an error is not expected to occur. In many cases this will be due to a bug, e.g. due to a
// defensive check failing
// defensive check failing.
// Note that if the IsUnexpected flag has not been set at all, this will still return false.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Felt it was worth adding a note here. Not changing this means this change should have no practical impact for someone relying on this function (we defaulted to false -> false, now we default to nil -> false).

Longer term I question if we really want this function, or perhaps we should have an equivalent Expected() function (that returns true only if the flag is set to false). But that risks making this more of a breaking change.

func (p *Error) Unexpected() bool {
if p.IsUnexpected != nil {
return *p.IsUnexpected
Expand Down
2 changes: 1 addition & 1 deletion errors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ func TestSetIsRetryable(t *testing.T) {

func TestSetIsUnexpected(t *testing.T) {
err := New("code", "message", nil)
assert.False(t, *err.IsUnexpected)
assert.Nil(t, err.IsUnexpected)

err.SetIsUnexpected(true)
assert.True(t, *err.IsUnexpected)
Expand Down
7 changes: 3 additions & 4 deletions factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,9 @@ func RateLimited(code, message string, params map[string]string) *Error {
// Builds a stack based on the current call stack
func errorFactory(code string, message string, params map[string]string) *Error {
err := &Error{
Code: ErrUnknown,
Message: message,
Params: map[string]string{},
IsUnexpected: &notUnexpected,
Code: ErrUnknown,
Message: message,
Params: map[string]string{},
}
if len(code) > 0 {
err.Code = code
Expand Down
Loading