Skip to content

feat(gqlerror): add Error.Is for errors.Is support - #449

Open
mvanhorn wants to merge 3 commits into
vektah:masterfrom
mvanhorn:feat/249-gqlerror-is-method
Open

feat(gqlerror): add Error.Is for errors.Is support#449
mvanhorn wants to merge 3 commits into
vektah:masterfrom
mvanhorn:feat/249-gqlerror-is-method

Conversation

@mvanhorn

Copy link
Copy Markdown

Summary

*gqlerror.Error now implements Is(error) bool, so two structurally-equal gqlerrors compare as equal under errors.Is (and cmpopts.EquateErrors, which is built on it). Previously errors.Is fell back to pointer identity, so two errors carrying identical fields but distinct instances never matched.

Background

The gqlerror package already ships a List.Is method, so Error.Is completes that pattern; #249 requested it and a maintainer replied "Excellent idea! PR welcome!". The method declines non-*Error targets (letting errors.Is keep walking the receiver's Unwrap chain) and, for a *Error target, compares the identifying fields the issue means by "the same fields": Message, Rule, Path, Locations, and Extensions. The private wrapped cause (Err) is intentionally left to the existing Unwrap chain rather than folded into Is, and the comparison is against the direct target only (not the target's unwrap chain), matching Go's errors.Is semantics. A nil receiver is guarded.

I have:

  • Added tests covering the bug / feature
  • Updated any relevant documentation (N/A - matches the existing undocumented List.Is/List.As style)

Closes #249

@coveralls

coveralls commented Jul 19, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 87.394% (+0.03%) from 87.366% — mvanhorn:feat/249-gqlerror-is-method into vektah:master

@StevenACoffman

Copy link
Copy Markdown
Collaborator

Thanks! I'm just thinking about this a bit.

The issue title sort of buries the fact that there's an intended ripple effect, so that List.Is behavior is also changed to structural rather than identity matching for gqlerror.List items.

gqlerror/error.go:111 has List.Is, which loops calling errors.Is(err, target) on each element. Because those elements are *Error, they would now carry the new structural Is. So errors.Is(someList, &gqlerror.Error{Message: "..."}) will now return true if any element structurally matches

Tensions I'm mulling over

reflect.DeepEqual treats nil and empty as different. Extensions: nil vs Extensions: map[string]any{} are not Is-equal; same for Path/Locations that are nil vs an empty slice. A matcher must replicate the nil-ness of these fields exactly, not just their contents. Sort of a footgun for test authors constructing target values. So kind of juggling these tensions:

  • Normalize inside Is (treat nil and empty slices/maps as equal): more forgiving, fewer caller surprises; aligns with "pull complexity downward."
  • Keep DeepEqual semantics: predictable and matches how reflect.DeepEqual behaves elsewhere.

… match

Make the structural matching of Error.Is and its ripple into List.Is
explicit via doc comments, and pin the List structural-match case with
a test, per review discussion.
@mvanhorn

Copy link
Copy Markdown
Author

Good catch on the ripple effect. I kept the structural List.Is behavior and made it explicit in d2e6bd9: doc comments on both Is methods spelling out the semantics, plus a List test covering the structural match case, so the change is deliberate and pinned by tests.

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.

Add Is(err error) bool to gqlerror to work with errors.Is

3 participants