docs: correct Nil processor description, errors.As guidance, and file structure#68
docs: correct Nil processor description, errors.As guidance, and file structure#68MasterOfBinary wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates documentation across several files (AGENTS.md, CLAUDE.md, README.md, doc.go, and source/doc.go) to clarify file structures, describe the Nil processor's timing behavior, warn about concurrent channel consumption, and explain how to use errors.As with pointer types. The feedback suggests simplifying the explanation of errors.As in AGENTS.md and CLAUDE.md to avoid non-idiomatic Go patterns like using new() with a pointer type.
| - Items with errors are tracked individually through the Error field | ||
| - Batch processing continues despite individual item errors | ||
| - Use `errors.As` to check error types (SourceError, ProcessorError) | ||
| - Use `errors.As` to check error types (SourceError, ProcessorError). The engine wraps errors as pointers (`&SourceError{...}`, `&ProcessorError{...}`), so `errors.As` must target the pointer type — e.g. `var se *batch.SourceError; errors.As(err, &se)` (equivalently `errors.As(err, new(*batch.SourceError))`). A value-target such as `var se batch.SourceError; errors.As(err, &se)` will not match. |
There was a problem hiding this comment.
The parenthetical (equivalently errors.As(err, new(*batch.SourceError))) is technically correct but highly non-idiomatic in Go.
In Go, the standard and idiomatic way to use errors.As with a pointer type is:
var se *batch.SourceError
errors.As(err, &se)Using new(*batch.SourceError) is rarely seen and can confuse readers because it returns a double pointer (**batch.SourceError), requiring extra dereferencing to access the error fields.
Additionally, the phrase "The engine wraps errors as pointers" could be more precisely stated as "The engine returns errors as pointers" or "The engine instantiates errors as pointers", since "wrapping" in Go errors typically refers to the Unwrap chain rather than pointer vs. value types.
Consider simplifying and refining this sentence:
- Use `errors.As` to check error types (SourceError, ProcessorError). The engine returns errors as pointers (`*SourceError`, `*ProcessorError`), so `errors.As` must target the pointer type — e.g., `var se *batch.SourceError; errors.As(err, &se)`. A value-target such as `var se batch.SourceError; errors.As(err, &se)` will not match.| - Items with errors are tracked individually through the Error field | ||
| - Batch processing continues despite individual item errors | ||
| - Use `errors.As` to check error types (SourceError, ProcessorError) No newline at end of file | ||
| - Use `errors.As` to check error types (SourceError, ProcessorError). The engine wraps errors as pointers (`&SourceError{...}`, `&ProcessorError{...}`), so `errors.As` must target the pointer type — e.g. `var se *batch.SourceError; errors.As(err, &se)` (equivalently `errors.As(err, new(*batch.SourceError))`). A value-target such as `var se batch.SourceError; errors.As(err, &se)` will not match. No newline at end of file |
There was a problem hiding this comment.
The parenthetical (equivalently errors.As(err, new(*batch.SourceError))) is technically correct but highly non-idiomatic in Go.
In Go, the standard and idiomatic way to use errors.As with a pointer type is:
var se *batch.SourceError
errors.As(err, &se)Using new(*batch.SourceError) is rarely seen and can confuse readers because it returns a double pointer (**batch.SourceError), requiring extra dereferencing to access the error fields.
Additionally, the phrase "The engine wraps errors as pointers" could be more precisely stated as "The engine returns errors as pointers" or "The engine instantiates errors as pointers", since "wrapping" in Go errors typically refers to the Unwrap chain rather than pointer vs. value types.
Consider simplifying and refining this sentence:
- Use `errors.As` to check error types (SourceError, ProcessorError). The engine returns errors as pointers (`*SourceError`, `*ProcessorError`), so `errors.As` must target the pointer type — e.g., `var se *batch.SourceError; errors.As(err, &se)`. A value-target such as `var se batch.SourceError; errors.As(err, &se)` will not match.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #68 +/- ##
=======================================
Coverage 96.73% 96.73%
=======================================
Files 12 12
Lines 368 368
=======================================
Hits 356 356
Misses 9 9
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fix #8: the Nil processor was described as "passes items through unchanged (for benchmarking)", which is wrong. Per processor/nil.go it sleeps for a configurable Duration (and can mark items cancelled via MarkCancelled). Corrected in CLAUDE.md, AGENTS.md, and README.md to match processor/doc.go. Fix #9: the engine emits wrapped errors as pointers (&SourceError{}, &ProcessorError{} in batch/batch.go), so errors.As must target the pointer type. Made the CLAUDE.md/AGENTS.md guidance explicit with a concrete example; README already used the pointer form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root doc.go: removed the "// Output:" marker from the package doc comment. It was not a real runnable Example and could mislead readers into thinking the snippet's output was verified; the illustrative code is kept with a plain comment. source/doc.go: the example drained out fully and then errs sequentially. That only works because Channel emits no errors; added a caution that data and errors should generally be consumed concurrently, since the Batch engine reads both channels at the same time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebased fix/doc-corrections onto master (ba2a756), which merged the single-use Batch API (#64) and the golangci-lint v2.12.2 install line (#78) into the same doc files this PR edits. The rebase auto-merged without textual conflicts, but doc.go required a genuine three-way reconciliation: master rewrote the package-doc snippet to the new API (errs, err := b.Go(...); if err != nil { log.Fatal(err) }; batch.IgnoreErrors(errs)) while this PR independently removed the misleading "// Output:" marker from that same block. Both edits were combined off the common ancestor, so the result keeps the new API and drops the marker — no manual conflict markers were ever produced. Reconciliation outcome: - Kept master's content: single-use semantics, Go's (<-chan error, error) signature, ErrBatchUsed/ErrNilSource, nil-Config default note, the removal of IDBufferSize, and the golangci-lint install command. - Re-applied this PR's still-valid corrections on top: Nil processor now documented as sleeping for a Duration (CLAUDE.md/AGENTS.md/README.md), errors.As pointer-target guidance (CLAUDE.md/AGENTS.md, verified against the &SourceError{}/&ProcessorError{} sends in batch/batch.go), File Structure refresh, and the doc.go / source/doc.go "// Output:" cleanups. - Dropped nothing as redundant: master and this PR touched disjoint concerns, so every correction this PR intended still applies. Verified post-rebase: go build ./..., go vet ./..., golangci-lint v2.12.2 run --timeout=3m (0 issues), go test ./... (all packages pass, including the runnable root Example), gofmt -l . empty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bcaca14 to
849789b
Compare
Summary
Fixes documentation drift found in a full-repo review.
Changes
Nilprocessor (HIGH drift):CLAUDE.md,AGENTS.md, andREADME.mddescribed it as a benchmarking pass-through, but it actually sleeps for a configurableDuration(and can mark items cancelled). Corrected to matchprocessor/doc.go.errors.Asguidance: clarified that the engine wraps errors as pointers, so callers must target the pointer type —var se *batch.SourceError; errors.As(err, &se). The value-target form does not match.CLAUDE.md's stale layout to reflect the actual tree (config.go,errors.go,constants.go,helpers.go, theexample_*_test.gofiles, concrete processor/source files).// Output:marker from the root package doc comment; added a concurrency caution to thesource/doc.goexample (data and errors should be consumed concurrently).Testing
go vet ./...,go build ./..., andgo test ./...all pass (documentation and package-doc comments only).Backwards compatibility
Documentation only.
Part of a 5-PR set from a full-repo review; file-disjoint and independently mergeable in any order.
🤖 Generated with Claude Code