fix: deep review improvements — bug fixes, docs, and test coverage#107
fix: deep review improvements — bug fixes, docs, and test coverage#107
Conversation
- Fix README typos: undefined var `err` -> `err1`, `.Tag()` -> `.Tags()` - Deep-copy tags in builder.copy() with slices.Clone to prevent shared-array bugs - Add panic recovery to lazyValueEvaluation (produces descriptive string on panic) - Replace lo.Assign with maps.Clone in builder.copy() (stdlib, idiomatic) - Document all global config vars (AutoTraceID, DereferencePointers, Local) in README - Document Wrapf + %w foot-gun behavior in README - Add logrus formatter tests (was zero coverage) - Add AsOops/AsError[T] helper tests - Add Format %s/%q verb coverage - Add benchmark suite for core operations
There was a problem hiding this comment.
Pull request overview
This PR improves correctness and robustness of the oops error builder/copy semantics, expands documentation around configuration and wrapping behavior, and adds missing tests/benchmarks (including coverage for the logrus formatter module).
Changes:
- Fix README examples/typos, document global config variables, and clarify
Wrapf+%wbehavior. - Make
OopsErrorBuilder.copy()safer by cloning tag slices and switching map copying to stdlib cloning; add panic recovery for lazy context evaluation. - Add new tests (format verbs,
AsOops/AsError[T], logrus formatter) and introduce a dedicated benchmarks module wired intogo.work.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Fixes examples and expands documentation for config and wrapping behavior. |
| builder.go | Updates builder copy behavior to clone slices/maps using stdlib helpers. |
| kv.go | Adds panic recovery to lazy value evaluation. |
| oops_test.go | Adds %s/%q formatting coverage for OopsError. |
| helpers_test.go | Adds tests for AsOops and AsError[T]. |
| go.work | Adds the new benchmarks module to the workspace. |
| loggers/logrus/go.mod | Adds test dependencies and changes how the root oops module is referenced. |
| loggers/logrus/go.sum | Updates module sums consistent with go.mod changes. |
| loggers/logrus/formatter_test.go | Introduces formatter tests to cover logrus integration. |
| benchmarks/go.mod | Adds a standalone module for benchmarks. |
| benchmarks/go.sum | Adds sums for benchmark module dependencies. |
| benchmarks/benchmark_test.go | Adds benchmark suite for core operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
51ac8ae to
db40ddb
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
==========================================
+ Coverage 84.47% 87.22% +2.75%
==========================================
Files 14 14
Lines 1037 1049 +12
==========================================
+ Hits 876 915 +39
+ Misses 135 104 -31
- Partials 26 30 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- fix unchecked errors on logger.Sync, AddObject, AddReflected
- replace unsafe type assertion with AsOops + ok check in logrus test
- normalize trailing whitespace on blank HTTP request lines in test
- add missing slog.KindAny case in builder
- modernize interface{} to any in zerolog formatter and kv test
- add nolint directives for intentional testifylint/paralleltest cases
…copy() maps.Clone(nil) returns nil, causing panics when callers like With, WithContext, User, and Tenant attempt to assign into those maps. Add nil-guards after cloning so the maps are always initialized.
err->err1,.Tag()->.Tags()