Skip to content

Add rows affected and query idx for ErrQueryAffectedMoreThanOneRow#121

Merged
MarcusGoldschmidt merged 2 commits into
mainfrom
goldschmidt/query-idx
Apr 25, 2026
Merged

Add rows affected and query idx for ErrQueryAffectedMoreThanOneRow#121
MarcusGoldschmidt merged 2 commits into
mainfrom
goldschmidt/query-idx

Conversation

@MarcusGoldschmidt
Copy link
Copy Markdown
Contributor

Description

  • Bug fix
  • New feature

Useful links:

@MarcusGoldschmidt MarcusGoldschmidt requested a review from a team April 25, 2026 16:48
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 25, 2026

Claude finished @MarcusGoldschmidt's task in 59s —— View job


PR Review

  • Gather context and read changed files
  • Review code changes
  • Post review feedback

Overall: This is a small, useful improvement. Adding the rows affected count and query index to the error will make debugging provisioning issues much easier — especially when multiple queries are involved. The sentinel error is correctly preserved via %w.

One suggestion on the error message format (pkg/bsql/query.go:395):

The current format string reads a bit awkwardly:

return fmt.Errorf("provisioning query affected rows %d > 1, query idx: %d error: %w", rowsAffected, idx, ErrQueryAffectedMoreThanOneRow)

Consider restructuring for clarity — something like:

return fmt.Errorf("provisioning query at index %d affected %d rows: %w", idx, rowsAffected, ErrQueryAffectedMoreThanOneRow)

This reads more naturally ("query at index 2 affected 5 rows") and drops the redundant error: prefix before the wrapped error (since %w already serves that role). The > 1 is implied by the sentinel error message. Fix this →

No issues with correctness:

  • errors.Is(err, ErrQueryAffectedMoreThanOneRow) will still work thanks to %w wrapping, which matters if callers want to match on this sentinel in the future (currently only ErrQueryAffectedZeroRows is checked via errors.Is in provisioning.go:77,128).
  • The idx variable is correctly scoped from the for idx, q := range queries loop.

LGTM with the minor formatting nit above — take it or leave it.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 25, 2026

Claude finished @MarcusGoldschmidt's task in 45s —— View job


PR Review

  • Gather context and read changed files
  • Review code changes
  • Post review feedback

Overall: Clean improvement — the updated error message at pkg/bsql/query.go:395 now includes both the query index and rows affected count, which will make debugging multi-query provisioning failures much easier.

I see the second commit (c02d76a) already adopted the cleaner format string from the previous review. The current message reads naturally:

return fmt.Errorf("provisioning query at index %d affected %d rows: %w", idx, rowsAffected, ErrQueryAffectedMoreThanOneRow)

Correctness check:

  • %w wrapping preserves the sentinel, so errors.Is(err, ErrQueryAffectedMoreThanOneRow) will still match. While this sentinel isn't currently checked via errors.Is anywhere (unlike ErrQueryAffectedZeroRows which is checked in provisioning.go:77,128), wrapping is the right call for forward compatibility.
  • idx is correctly scoped from the for idx, q := range queries loop at line 378.

No issues found. LGTM.

@MarcusGoldschmidt MarcusGoldschmidt merged commit 7097037 into main Apr 25, 2026
8 checks passed
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.

3 participants