Update error handling as per Go 1.13 guidelines

[mbalayil]

Fixes 1846

1. Updated 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 all the affected tests.
3. Implemented Is(error) bool interface for all the required errors (ErrorResponse, RateLimitError, AcceptedError and AbuseRateLimitError). All the individual fields are directly compared. However, for the *http Response field, only StatusCode is being compared. If the other fields of the http.Response struct need to be compared, please let me know.

[codecov]

Codecov Report

Merging #1854 (f4057c7) into master (d711542) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1854      +/-   ##
==========================================
+ Coverage   97.63%   97.64%   +0.01%     
==========================================
  Files         105      105              
  Lines        6720     6762      +42     
==========================================
+ Hits         6561     6603      +42     
  Misses         86       86              
  Partials       73       73              

Impacted Files	Coverage Δ
github/github.go	97.01% <100.00%> (+0.38%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d711542...f4057c7. Read the comment docs.

[gmlewis]

Thank you, @mbalayil !
Please address the comments below.

[mbalayil]

Have updated the PR with more tests to improve coverage. Please take a look.

[gmlewis]

Thank you, @mbalayil !
LGTM.

Awaiting second LGTM before merging.

[wesleimp]

💙

[gmlewis]

Thank you, @wesleimp!

@willnorris - are you OK with this implementation? I'll wait until we hear from you... no rush... I just want to make sure since you had some comments on the thread.

[willnorris]

@willnorris - are you OK with this implementation?

My biggest concern, just looking at this from a high level, is that this is a LOT of work and extra code just to be able to call errors.Is in a couple of test cases. If having these errors be comparable in this way has real value outside of just these tests, then that may be fine, and it may be worth the effort. But if it's just to have marginally cleaner tests (not calling reflect.DeepEqual), I find it harder to justify.

The whole point of the new go1.13 error handling is that the Is and As functions support wrapped errors. Both of them will intelligently unwrap errors to try and find a match. But we're not wrapping errors. Our errors are just values, like any other value in Go. So honestly, there's no advantage of using errors.Is versus the go-cmp package (in our tests, I mean).

I really appreciate all the effort @mbalayil put into this... really, I do. But looking at what it ended up taking in the end, I'm not sure that this is the right solution for this problem. (Unless there's some other use case that this is satisfying, outside of those two test cases.)

[gmlewis]

Correct me if I'm wrong, @willnorris , but won't the availability of the Is methods (added by tihs PR) make it easier for the users of this package to wrap their errors and still test if the base error is one of the types defined in this repo?

In other words, doesn't the work here make error handler easier for future users of this package (and also make the usage more inline with the Go 1.13 Error Handling blog post)?

[willnorris]

Yeah, that's a good point. For the sentinel errors we have, I think that will be very useful (but also doesn't require a custom Is method). Given how a couple of these errors are tied to very specific http requests/responses, I think the full equality check is going to be of very limited value, even for downstream users, but am happy to be proven wrong.

[gmlewis]

I think the full equality check is going to be of very limited value, even for downstream users, but am happy to be proven wrong.

So do you think it would be more useful without the equality checks?

Or should we run with this for awhile until we get feedback and then adjust accordingly?

[willnorris]

Looking more at the examples I was concerned about, they seem to be more limited than I thought. I was mostly focusing on the CreatedAt timestamp that exists in some errors. I thought that was more common, but I now see it is only for HTTP 451 errors. For those errors, an equality check will almost never match in the real world because you won't know the creation date ahead of time.

I guess I'd need to look at some more real-world errors to see how likely it would be for people to really be doing equality checks. My guess is that most go-github users that want to wrap our errors and eventually test for error types will use errors.As (which we don't have to do anything for).

I know that doesn't directly answer your question, but I guess I'm discovering that my once somewhat-strongly-held view is softening considerably, so I can go either way on this now.

[gmlewis]

OK, sounds good. Thanks, Will!
Let's run with this and see what kind of feedback we get. 😂
Merging.