Follow recommended Go 1.13 error handling guidelines

[gmlewis]

In a recent code review, we felt that this repo should use the recommended Go 1.13 error handling guidelines.

Specifically, we should never use reflect.DeepEqual for checking errors (even in tests) and would ideally use the "new" error.Is functionality provided in Go 1.13.

Additionally, we should update the README.md to reflect that it is no longer Go 1.9 that we require, but instead we need a minimum of Go 1.13.

[mbalayil]

I would like to work on this issue.

[gmlewis]

Thank you, @mbalayil ! It is yours.

[mbalayil]

There are 6 places in github/github_test.go where reflect.DeepEqual is used for error comparisons. Switching it to errors.Is in TestDo_nilContext is straightforward.

@@ -705,7 +708,7 @@ func TestDo_nilContext(t *testing.T) {
        req, _ := client.NewRequest("GET", ".", nil)
        _, err := client.Do(nil, req, nil)

-       if !reflect.DeepEqual(err, errors.New("context must be non-nil")) {
+       if !errors.Is(err, errNonNilContext) {
                t.Errorf("Expected context must be non-nil error")
        }
 }

where errNonNilContext is defined in github/github.go.

However for the rest of the 5 instances where reflect.DeepEqual is used, the errors being tested are not sentinel errors.
We can either implement Is(error) bool interface for ErrorResponse, RateLimitError and AbuseRateLimitError (possibly using github.com/google/go-cmp) and utilize errors.Is in the corresponding tests or use github.com/google/go-cmp directly to compare the expected and actual values as shown below.

@@ -1097,7 +1100,11 @@ func TestCheckResponse(t *testing.T) {
                        CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)},
                },
        }
-       if !reflect.DeepEqual(err, want) {
+       opts := []cmp.Option{
+               cmp.AllowUnexported(bytes.Buffer{}),
+               cmp.AllowUnexported(http.Request{}),
+       }
+       if !cmp.Equal(err, want, opts...) {
                t.Errorf("Error = %#v, want %#v", err, want)
        }
 }

Which method is preferred ? Or is there another alternative ?

[gmlewis]

My opinion is to implement Is(target error) bool for each of those errors (and maybe also for AcceptedError).

[mbalayil]

Can I use github.com/google/go-cmp for the implementation of the Is function ?

[gmlewis]

Can I use github.com/google/go-cmp for the implementation of the Is function ?

Do you have a link? I'm not seeing any Is method on this page: https://pkg.go.dev/github.com/google/go-cmp/cmp

Shouldn't it be as simple as this:

func (e *AcceptedError) Is(target error) bool {
    _, ok := target.(*AcceptedError)
    return ok
}

and then it would be used like this:

if errors.Is(err, &AcceptedError{}) {
    // ...
}

or am I missing something?

[willnorris]

Isn't errors.Is intended to be an equality check, not just a type check (with the added feature of checking the full error chain)?

If so, AcceptedError.Is() should probably consider the raw field as well... I worry about unintended consequences elsewhere if we ignore it. If all we care about in this test is the type, then we should just do a type check directly in the test.

[gmlewis]

That makes sense. Sounds good, @willnorris .

[mbalayil]

1. Updated the README.md to reflect that it is no longer Go 1.9 that we require, but instead we need a minimum of Go 1.13.
2. Replaced reflect.DeepEqual with errors.Is in TestDo_nilContext.
3. Implemented Is function for AcceptedError.

Will update a PR soon with these partial fixes.
However, for ErrorResponse, RateLimitError and AbuseRateLimitError, I am still looking for ways to implement the Is function without using github.com/google/go-cmp. If you have any suggestions on comparing the individual fields of a struct with nested fields, let me know.

[gmlewis]

Hi @mbalayil - go ahead and provide a PR with your proposed solution (using go-cmp) so we can take a look at it, please.

[mbalayil]

provide a PR with your proposed solution (using go-cmp)

"It is intended to only be used in tests, as performance is not a goal and it may panic if it cannot compare the values. Its propensity towards panicking means that its unsuitable for production environments where a spurious panic may be fatal."

In https://pkg.go.dev/github.com/google/go-cmp/cmp#pkg-overview, it is mentioned that this package is better not used in production code. So I have created the PR by directly comparing the values. However, for the http.Response struct, only StatusCode is being compared. If the other fields of the http.Response struct need to be compared, please let me know.

Apologies for the delay. It took me some time to figure all this out.

[gmlewis]

That makes sense to me. Let's try this out unless we hear other feedback.