Skip to content

Migrate error comparisons to errors.Is/errors.As#84

Draft
bobheadxi wants to merge 1 commit into
masterfrom
errors-is-as-migration-7f3a9c
Draft

Migrate error comparisons to errors.Is/errors.As#84
bobheadxi wants to merge 1 commit into
masterfrom
errors-is-as-migration-7f3a9c

Conversation

@bobheadxi

Copy link
Copy Markdown
Member

This change migrates direct error comparisons and error type assertions to the
modern errors package idioms:

  • err == SentinelErr / err != SentinelErr -> errors.Is(err, SentinelErr) / !errors.Is(...)
    for sentinels such as sql.ErrNoRows, context.Canceled, context.DeadlineExceeded,
    http.ErrServerClosed, io.ErrUnexpectedEOF, os.ErrNotExist, etc.
  • Error type assertions and switch err.(type) classification -> errors.As.

Excluded by design: io.EOF comparisons (idiomatic in reader loops) and _test.go files.

This makes error checks robust to wrapped errors (fmt.Errorf("...: %w", err)).

Generated by a Sourcegraph batch change.

Created by Sourcegraph batch change robert/58006530-3ddc-4c11-ad42-8d8eed8bc63f.

@bobheadxi bobheadxi marked this pull request as ready for review June 15, 2026 18:23
Comment thread diff/parse.go
var e0 *ParseError
if errors.As(err, &e0) {
var e *ErrBadHunkLine
if errors.As(e0.Err, &e) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should add the Unwrap method to ParseError in this PR then we can simplify this and other places so we don't first have to check for ParseError

Comment thread diff/parse.go
if pe, ok := err.(*ParseError); ok && pe.Err == ErrExtendedHeadersEOF {
var pe *ParseError
var oe OverflowError
if errors.As(err, &pe) && errors.Is(pe.Err, ErrExtendedHeadersEOF) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

code like this make me nervous in general, but I believe its correct. IE we are relying on the first condition to mutate pe so that the second condition doesn't nil panic on pe.Err

@bobheadxi

Copy link
Copy Markdown
Member Author

@keegancsmith sorry I hit publish on this while practicing a demo, this is just a test case 🙇

@bobheadxi bobheadxi marked this pull request as draft June 17, 2026 19:56
@keegancsmith

Copy link
Copy Markdown
Member

@bobheadxi I thought that might be the case, but being able to handle follow-up comments seems like a good test case for testing BCA :)

Do you plan on following up with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants