feat: initial implementation of error adr - #1
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughA new Go library named Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Code
participant Tmpl as ErrorTemplate
participant Err as TrogonError
App->>Tmpl: NewErrorTemplate(domain, reason, options...)
App->>Tmpl: NewError(options...)
Tmpl->>Err: Create TrogonError instance with template defaults
App->>Err: (optionally) Apply ErrorOptions (metadata, stack, etc.)
Err-->>App: Returns TrogonError (implements error)
App->>Err: Use error (log, return, wrap, etc.)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
5629ccc to
d06daf5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
errorformatter.go (1)
56-56: Remove unnecessary empty line.There's an unnecessary empty line that should be removed for cleaner code.
fmt.Fprintf(sb, "\n retry_info: %s", retryStr) } -template_test.go (1)
286-286: Fix formatting in example outputThe output has a formatting issue where the subject line is concatenated with the help link description.
- // subject: /users/123- User API Docs: https://docs.myapp.com/users + // subject: /users/123 + // help: + // - User API Docs: https://docs.myapp.com/users
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.github/workflows/ci.yml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)doc.go(1 hunks)error.go(1 hunks)error_test.go(1 hunks)errorformatter.go(1 hunks)example_test.go(1 hunks)go.mod(1 hunks)template_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
error_test.go (1)
error.go (42)
NewError(310-327)WithCode(330-334)Unknown(21-21)SpecVersion(12-12)Code(15-15)VisibilityInternal(57-57)Visibility(53-53)WithMetadataValue(352-356)VisibilityPublic(61-61)Metadata(88-88)WithSubject(366-370)WithID(373-377)WithTime(380-384)WithSourceID(388-392)WithHelpLink(402-412)Help(74-77)Internal(43-43)WithStack(423-425)DebugInfo(91-96)WithStackDetail(428-430)WithStackDepth(433-441)WithStackDetailDepth(444-452)WithCause(514-518)WithMessage(337-341)NotFound(27-27)TrogonError(117-152)WithRetryTime(504-511)RetryInfo(109-114)WithRetryInfoDuration(493-500)Cancelled(19-19)InvalidArgument(23-23)DeadlineExceeded(25-25)AlreadyExists(29-29)PermissionDenied(31-31)Unauthenticated(49-49)ResourceExhausted(33-33)FailedPrecondition(35-35)Aborted(37-37)OutOfRange(39-39)Unimplemented(41-41)Unavailable(45-45)DataLoss(47-47)
errorformatter.go (1)
error.go (1)
TrogonError(117-152)
example_test.go (1)
error.go (11)
NewError(310-327)WithCode(330-334)NotFound(27-27)WithMetadataValue(352-356)VisibilityPublic(61-61)NewErrorTemplate(769-783)TemplateWithCode(786-790)WithSubject(366-370)Internal(43-43)WithMessage(337-341)WithCause(514-518)
template_test.go (1)
error.go (25)
NewErrorTemplate(769-783)TemplateWithCode(786-790)NotFound(27-27)TemplateWithVisibility(798-802)VisibilityPublic(61-61)NewError(310-327)Code(15-15)Visibility(53-53)WithMetadataValue(352-356)WithSubject(366-370)Metadata(88-88)Unauthenticated(49-49)TemplateWithHelpLink(810-820)Help(74-77)ResourceExhausted(33-33)Internal(43-43)TemplateWithMessage(792-796)TrogonError(117-152)WithTime(380-384)VisibilityInternal(57-57)WithVisibility(359-363)InvalidArgument(23-23)DebugInfo(91-96)WithStackDetail(428-430)WithRetryInfoDuration(493-500)
🪛 LanguageTool
README.md
[uncategorized] ~243-~243: Possible missing preposition found.
Context: ...nes - Keep metadata values as strings - Include relevant debugging information ### Vis...
(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~253-~253: Possible missing preposition found.
Context: ...h to avoid overly complex error trees - Include the most relevant information at each l...
(AI_HYDRA_LEO_MISSING_TO)
[style] ~286-~286: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request.
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (8)
LICENSE (1)
3-3: LGTM!Copyright holder update is appropriate and aligns with the project ownership mentioned in the AI summary.
go.mod (1)
5-11: Dependencies look appropriate.The testify dependency and its indirect dependencies are standard choices for Go testing.
.github/workflows/ci.yml (1)
1-25: Well-structured CI workflow.The workflow follows best practices with appropriate triggers, Go version matrix that matches go.mod, and essential quality checks (tests + vet).
doc.go (1)
1-63: Excellent package documentation.The documentation is comprehensive, well-structured, and provides clear examples covering all major features. It follows Go documentation conventions and effectively communicates the package's capabilities.
errorformatter.go (1)
5-6: slices.Sorted compatibility confirmedThe
slices.Sortedfunction was introduced in Go 1.23.0, and yourgo.modspecifies Go 1.24.2—so this usage is fully supported. No changes needed.README.md (2)
36-37: Fix incorrect method names in documentationThe documentation references methods that don't exist in the actual implementation. Based on the code in
error.go, the correct method names should be used.- trogonerror.WithCodeMessage(trogonerror.NotFound), - trogonerror.WithMetadataEntry("user_id", "123")) + trogonerror.WithCode(trogonerror.NotFound), + trogonerror.WithMetadataValue("user_id", "123", trogonerror.VisibilityPublic))The same corrections need to be applied throughout the README:
- Replace all instances of
WithCodeMessagewithWithCode- Replace all instances of
WithMetadataEntrywithWithMetadataValue(which requires a visibility parameter)Also applies to: 57-58, 61-61, 76-77
Likely an incorrect or invalid review comment.
73-95: Remove references to unimplemented featuresThe documentation references several features that don't exist in the current implementation:
- Builder pattern (
NewErrorBuilder)- Quick constructors (
QuickError,ValidationError,NotFoundError,InternalError)- Helper functions (
IsTrogonError,AsTrogonError,WrapError)- Methods like
DefaultMessage()on Code typeThese should either be implemented or removed from the documentation to avoid confusion.
Also applies to: 130-130, 183-183, 209-220
Likely an incorrect or invalid review comment.
error.go (1)
534-588: Well-implemented Copy-on-Write semanticsThe implementation correctly achieves the Copy-on-Write semantics mentioned in the PR objectives through the
copy()method andWithChanges()function. This ensures safe concurrency without shared memory mutation.
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/ci.yml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)doc.go(1 hunks)error.go(1 hunks)error_test.go(1 hunks)example_test.go(1 hunks)go.mod(1 hunks)template_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- LICENSE
- go.mod
- doc.go
- example_test.go
- error_test.go
- template_test.go
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~243-~243: Possible missing preposition found.
Context: ...nes - Keep metadata values as strings - Include relevant debugging information ### Vis...
(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~253-~253: Possible missing preposition found.
Context: ...h to avoid overly complex error trees - Include the most relevant information at each l...
(AI_HYDRA_LEO_MISSING_TO)
[style] ~286-~286: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request.
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (5)
error.go (5)
1-11: Package setup looks good.Clean package declaration with appropriate imports for the error handling functionality.
521-544: Excellent stack trace implementation.Well-designed stack capture using Go's runtime package with proper frame skipping and depth control.
600-643: Solid copy-on-write implementation.The deep copy logic properly handles all field types and implements safe copy-on-write semantics as specified in the PR objectives.
822-921: Well-designed error template system.The template pattern provides excellent reusability while maintaining type safety and following Go idioms. Good separation between template definition and instance creation.
1-921: Excellent architecture that meets PR objectives.The implementation successfully delivers on all stated goals:
- ✅ Copy-on-write semantics via deep copying
- ✅ Safe concurrency through immutability
- ✅ High performance with efficient patterns
- ✅ YAGNI principle with extensible design
- ✅ Full Go error interface compatibility
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
README.md (3)
149-152:InternalErrorandWithMetadataEntrystill missing from the public APIThe chaining example re-introduces
InternalErrorand the deprecatedWithMetadataEntryhelper:dbErr := trogonerror.InternalError(..., trogonerror.WithMetadataEntry("host", "db.example.com"))Unless those helpers have now been added, please swap to the canonical constructor & option set, e.g.
-dbErr := trogonerror.InternalError("com.myapp.database", "CONNECTION_FAILED", - trogonerror.WithMetadataEntry("host", "db.example.com")) +dbErr := trogonerror.NewError("com.myapp.database", "CONNECTION_FAILED", + trogonerror.WithCode(trogonerror.Internal), + trogonerror.WithMetadataValue("host", "db.example.com", trogonerror.VisibilityPrivate))
163-167: Retry-info snippet still uses the removedWithMetadataEntrySame discrepancy as above; update to
WithMetadataValue(and add visibility):- trogonerror.WithRetryInfo(60 * time.Second), - trogonerror.WithMetadataEntry("limit", "100"), - trogonerror.WithMetadataEntry("window", "1m")) + trogonerror.WithRetryInfo(60 * time.Second), + trogonerror.WithMetadataValue("limit", "100", trogonerror.VisibilityPublic), + trogonerror.WithMetadataValue("window", "1m", trogonerror.VisibilityPublic))
82-95: Stale “quick-constructor” APIs are still referenced
QuickError,ValidationError,NotFoundErrorandWithMetadataEntrywere already called out in the previous review as non-existent. They are still present in this section, so the examples will not compile against the current library.-// Quick error creation -err := trogonerror.QuickError("com.myapp.auth", "INVALID_TOKEN", trogonerror.Unauthenticated) - -// Specialized error types with functional options -validationErr := trogonerror.ValidationError("com.myapp.users", "INVALID_EMAIL", - trogonerror.WithSubject("/data/email"), - trogonerror.WithMetadataEntry("field", "email")) - -notFoundErr := trogonerror.NotFoundError("com.myapp.users", "USER_NOT_FOUND", - trogonerror.WithMetadataEntry("user_id", "123")) +// Quick error creation +err := trogonerror.NewError("com.myapp.auth", "INVALID_TOKEN", + trogonerror.WithCode(trogonerror.Unauthenticated)) + +// Specialized errors with functional options +validationErr := trogonerror.NewError("com.myapp.users", "INVALID_EMAIL", + trogonerror.WithCode(trogonerror.InvalidArgument), + trogonerror.WithSubject("/data/email"), + trogonerror.WithMetadataValue("field", "email", trogonerror.VisibilityPublic)) + +notFoundErr := trogonerror.NewError("com.myapp.users", "USER_NOT_FOUND", + trogonerror.WithCode(trogonerror.NotFound), + trogonerror.WithMetadataValue("user_id", "123", trogonerror.VisibilityPublic))Either implement these helper functions or update the docs as suggested so the code remains truthful and copy-pastable.
🧹 Nitpick comments (1)
README.md (1)
125-133: Example missestimeimport – won’t compile as writtenThe snippet calls
time.Now()but doesn’t import the package. Add it to keep the copy-paste experience friction-free.import ( "github.com/TrogonStack/trogonerror" + "time" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)error_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- error_test.go
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~237-~237: Possible missing preposition found.
Context: ...nes - Keep metadata values as strings - Include relevant debugging information ### Vis...
(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~247-~247: Possible missing preposition found.
Context: ...h to avoid overly complex error trees - Include the most relevant information at each l...
(AI_HYDRA_LEO_MISSING_TO)
[style] ~283-~283: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request.
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (1)
README.md (1)
235-238: Tiny grammar nit“Include relevant debugging information” → “Include relevant debugging information to …”
[ suggest_nitpick ]
ldemailly
left a comment
ldemailly
left a comment
There was a problem hiding this comment.
mostly surface comments - if any thing on substance it seems some/a lot of the code could be generated. and also not quite sure what this does overall
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
1-230: Turn README snippets into Go examples to prevent driftConsider moving the showcased code into
example_test.gofiles. Runninggo testwill compile every snippet, catching API drift automatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🔇 Additional comments (4)
README.md (4)
40-48: Double-check accessor method names in the quick-start snippetThe example prints
err.Domain()anderr.Reason(). Verify that theTrogonErrorimplementation actually exports methods with those exact names (and signatures). If the API instead exposesGetDomain,ErrorDomain, or public fields, this code will not compile.
60-75: Validate template helpers andNewErrorconstructorThe template example relies on:
TemplateWithCodeTemplateWithHelpLinkErrUserNotFound.NewError(...)Please confirm all three identifiers exist and are exported exactly as written; earlier revisions used
New, which caused build failures.
187-192: EnsureRetryInfo/RetryOffsetchaining matches realityThe retry example assumes:
retryInfo := err.RetryInfo() retryOffset := retryInfo.RetryOffset()Double-check that
RetryInfo()returns a type exposingRetryOffset()(notOffset,GetOffset, or a field). Adjust the docs or API for consistency.
225-229: ConfirmWrapErrorhelper signature
WrapError(originalErr, domain, reason, opts...)must exist exactly as shown; earlier APIs usedWrap. Mismatched names or parameter order will break the example.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
114-116:CancelledvsCanceled– verify the canonical constant name.gRPC & Google-style error codes use the American spelling
Canceled.
If the implementation follows that convention, the README should listtrogonerror.Canceledto prevent confusion.- trogonerror.Cancelled // Operation was cancelled + trogonerror.Canceled // Operation was canceled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.yml(1 hunks)README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
🔇 Additional comments (3)
README.md (3)
150-156: Confirmed:WithStack()&VisibilityInternalare present.
Both identifiers are defined inerror.go(e.g.,WithStack()at line 489,VisibilityInternalat line 58) and exercised in tests.
60-74: All referenced API symbols are implementedVerified that the following symbols exist in
error.go:
func NewErrorTemplate(domain, reason string, options ...TemplateOption) *ErrorTemplate(line 835)func TemplateWithCode(code Code) TemplateOption(line 852)func TemplateWithHelpLink(description, url string) TemplateOption(line 876)func (et *ErrorTemplate) NewError(options ...ErrorOption) *TrogonError(line 889)No further changes needed.
181-192: No changes needed:RetryOffset()matches implementationThe
RetryInfostruct inerror.godefines the following accessors:
func (r *RetryInfo) RetryOffset() *time.Durationfunc (r *RetryInfo) RetryTime() *time.TimeThere is no
RetryDelay()method, so the README example is accurate as written.
There was a problem hiding this comment.
Pull Request Overview
This PR adds the initial implementation of the Straw Hat ADR–compliant error library, introducing core error types, templating, and full test coverage.
- Defines
TrogonErrorwith functional options, code mappings, visibility, metadata, debug info, retry info, and wrapping (WrapError). - Introduces
ErrorTemplatefor reusable error definitions and instance creation. - Adds comprehensive tests, examples, documentation (README and doc.go), license update, and CI/dependabot workflows.
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| error.go | Implements TrogonError, error options, ErrorTemplate, and copy/mutation logic. |
| template_test.go | Tests for ErrorTemplate behavior, help links, concurrency safety, and matching. |
| error_test.go | Tests for TrogonError core behavior: metadata, debug info, stack, retry, wrapping, and HTTP code mapping. |
| example_test.go | Usage examples for NewError, templates, causes, and code methods. |
| go.mod | Module path, Go version, and dependencies. |
| README.md | Updated project overview, installation, usage examples, and code snippets. |
| doc.go | Package-level documentation comments and usage instructions. |
| LICENSE | Updated copyright holder. |
| .github/workflows/ci.yml | CI configuration with Go tests, vet, and linter. |
| .github/dependabot.yml | Dependabot configuration for monthly updates. |
Comments suppressed due to low confidence (1)
README.md:218
- The method
ToInt()does not exist onCode. UseHttpStatusCode()instead.
fmt.Println(code.ToInt()) // 404
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
error.go (3)
202-211: Fix inconsistent help links formatting.The help section lacks proper structure with missing section header and inconsistent newline handling.
213-221: Fix debug info formatting inconsistencies.The debug info section needs proper formatting structure to maintain consistency with other sections.
613-613: Deep copy retryInfo to avoid mutation sharing.The retryInfo field contains mutable pointers that should be deep copied.
- retryInfo: e.retryInfo, + retryInfo: nil, // Will be deep copied below if non-nilThen add after line 639:
if e.retryInfo != nil { clonedErr.retryInfo = &RetryInfo{} if e.retryInfo.retryOffset != nil { offset := *e.retryInfo.retryOffset clonedErr.retryInfo.retryOffset = &offset } if e.retryInfo.retryTime != nil { t := *e.retryInfo.retryTime clonedErr.retryInfo.retryTime = &t } }
🧹 Nitpick comments (1)
error.go (1)
534-534: Add newline prefix to stack entries for better readability.Stack trace entries should each be on a new line for better debugging experience.
- entry := fmt.Sprintf("%s:%d %s", frame.File, frame.Line, frame.Function) + entry := fmt.Sprintf("\n %s:%d %s", frame.File, frame.Line, frame.Function)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.github/dependabot.yml(1 hunks).github/workflows/ci.yml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)doc.go(1 hunks)error.go(1 hunks)error_format_test.go(1 hunks)error_test.go(1 hunks)example_test.go(1 hunks)examples_comprehensive_test.go(1 hunks)go.mod(1 hunks)template_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/ci.yml
- error_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- LICENSE
- .github/dependabot.yml
- go.mod
- doc.go
- example_test.go
- README.md
- error_format_test.go
- template_test.go
🔇 Additional comments (3)
examples_comprehensive_test.go (2)
10-33: Well-structured basic example!This example effectively demonstrates the fundamental usage of the trogonerror package with clear output formatting.
275-276: Good practice: Sorting metadata keys for consistent output!Using a predefined order for metadata keys ensures deterministic output in examples and tests.
error.go (1)
908-920: Well-designed template matching!The Is() method elegantly allows checking if an error was created from a template without requiring the template to implement the error interface. Good API design.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
error.go (3)
155-164: Inconsistent help links formatting remains unaddressed.The help links section still has the formatting issues identified in past reviews - missing section header, inconsistent newline handling, and unnecessary trailing newlines.
166-174: Debug info formatting inconsistencies remain unaddressed.The debug info section lacks proper formatting structure and consistency with other sections, as previously identified.
566-566: Incomplete deep copy implementation for retryInfo.The copy() method still shares the retryInfo pointer between original and cloned errors, which could lead to unintended mutation sharing. This violates the Copy-on-Write semantics mentioned in the PR objectives.
Apply this fix to ensure proper deep copying:
- retryInfo: e.retryInfo, + retryInfo: nil, // Will be deep copied below if non-nilThen add this after the existing deep copy sections (around line 592):
+ // Deep copy retryInfo to prevent mutation sharing + if e.retryInfo != nil { + retryInfoCopy := &RetryInfo{} + if e.retryInfo.retryOffset != nil { + offset := *e.retryInfo.retryOffset + retryInfoCopy.retryOffset = &offset + } + if e.retryInfo.retryTime != nil { + time := *e.retryInfo.retryTime + retryInfoCopy.retryTime = &time + } + clonedErr.retryInfo = retryInfoCopy + }
🧹 Nitpick comments (2)
error.go (2)
473-496: Potential performance issue with stack trace capture.The
captureStackTracefunction allocates a slice withmaxDepthcapacity but may only use a fraction of it. For high-frequency error creation with large default depths (32), this could lead to unnecessary memory allocation.Consider optimizing memory usage:
func captureStackTrace(skip, maxDepth int) []string { if maxDepth <= 0 { maxDepth = 32 // Reasonable default } - var pcs = make([]uintptr, maxDepth) + var pcs = make([]uintptr, maxDepth) n := runtime.Callers(skip, pcs[:]) + pcs = pcs[:n] // Trim to actual size frames := runtime.CallersFrames(pcs[:n]) var stackEntries []string for { frame, more := frames.Next() entry := fmt.Sprintf("%s:%d %s", frame.File, frame.Line, frame.Function) stackEntries = append(stackEntries, entry) if !more { break } } return stackEntries }
703-723: Consider adding validation to public getters.The getter methods directly return internal fields without any validation. While this follows Go conventions, consider whether defensive copies should be returned for mutable fields like
MetadataandCauses.For better encapsulation, consider returning defensive copies:
-func (e TrogonError) Metadata() Metadata { return e.metadata } -func (e TrogonError) Causes() []*TrogonError { return e.causes } +func (e TrogonError) Metadata() Metadata { + if e.metadata == nil { + return nil + } + result := make(Metadata, len(e.metadata)) + for k, v := range e.metadata { + result[k] = v + } + return result +} +func (e TrogonError) Causes() []*TrogonError { + if len(e.causes) == 0 { + return nil + } + result := make([]*TrogonError, len(e.causes)) + copy(result, e.causes) + return result +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.github/dependabot.yml(1 hunks).github/workflows/ci.yml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)doc.go(1 hunks)error.go(1 hunks)error_format_test.go(1 hunks)error_test.go(1 hunks)example_test.go(1 hunks)go.mod(1 hunks)template_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- LICENSE
- go.mod
- .github/dependabot.yml
- .github/workflows/ci.yml
- doc.go
- error_test.go
- error_format_test.go
- template_test.go
- example_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ldemailly
PR: TrogonStack/trogonerror#1
File: go.mod:3-3
Timestamp: 2025-07-02T22:05:52.952Z
Learning: For Go libraries, it's better to use a slightly older stable Go version (like 1.23 instead of the latest 1.24) to ensure broader compatibility with downstream users who may not have upgraded to the newest Go version yet.
🔇 Additional comments (6)
error.go (2)
19-36: Excellent use of iota for error code constants.The error code constants are well-defined and follow standard practices. The mapping to standardized error codes provides good interoperability.
325-346: Well-designed functional options pattern.The constructor uses the functional options pattern effectively, providing flexibility while maintaining reasonable defaults. The initialization is clean and follows Go best practices.
README.md (4)
1-10: Excellent comprehensive introduction.The README provides a clear, comprehensive introduction that effectively communicates the library's purpose, benefits, and target audience. The multi-paragraph structure addresses the what, why, and who very effectively.
31-42: Good basic usage example structure.The basic usage example demonstrates the core functionality clearly and follows logical patterns. The import statement and error creation syntax appear consistent with the implementation.
46-58: Well-designed template example.The production template example effectively demonstrates the recommended pattern for reusable error definitions. This aligns well with the template system implemented in error.go.
40-41: Quick Start example is accurate;Domain()andReason()are valid methodsThe
TrogonErrortype defines the following getter methods inerror.go(lines 711–712), so the README’s use oferr.Domain()anderr.Reason()is correct:
func (e TrogonError) Domain() string { return e.domain }func (e TrogonError) Reason() string { return e.reason }No changes needed to the documentation.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
error.go (4)
154-163: Fix inconsistent help links formatting.The help links formatting has several issues:
- Missing newline prefix before help links section
- Inconsistent newline handling in the loop
- Unnecessary trailing newline
if e.help != nil && len(e.help.links) > 0 { + sb.WriteString("\n help:") for i, link := range e.help.links { - if i > 0 { - sb.WriteString("\n") - } - fmt.Fprintf(sb, "- %s: %s", link.description, link.url) + fmt.Fprintf(sb, "\n - %s: %s", link.description, link.url) } - sb.WriteString("\n") }
165-173: Fix debug info formatting inconsistencies.The debug info section lacks proper formatting structure and consistency with other sections.
if e.debugInfo != nil { + sb.WriteString("\n debug_info:") if e.debugInfo.detail != "" { - sb.WriteString(e.debugInfo.detail) + fmt.Fprintf(sb, "\n detail: %s", e.debugInfo.detail) } for _, entry := range e.debugInfo.stackEntries { - sb.WriteString(entry) + fmt.Fprintf(sb, "\n %s", entry) } }
559-560: Potential memory sharing issues with time and localizedMessage pointers.The copy() method shares pointers for
timeandlocalizedMessagefields. While these might be intended as immutable, sharing pointers violates true copy-on-write semantics and could lead to subtle bugs if the pointed-to values are ever modified.Consider deep copying these fields for true copy-on-write semantics:
- time: e.time, - localizedMessage: e.localizedMessage, + time: nil, // Will be deep copied below if non-nil + localizedMessage: nil, // Will be deep copied below if non-nilAdd deep copy logic after the help and debugInfo copying (after line 586):
+ // Deep copy time pointer + if e.time != nil { + timeCopy := *e.time + clonedErr.time = &timeCopy + } + + // Deep copy localizedMessage + if e.localizedMessage != nil { + clonedErr.localizedMessage = &LocalizedMessage{ + locale: e.localizedMessage.locale, + message: e.localizedMessage.message, + } + }
170-172: Fix stack trace formatting in Error() method.The stack entries are being appended directly without proper formatting, which could result in malformed output when stack traces are present.
- for _, entry := range e.debugInfo.StackEntries() { - sb.WriteString(entry) - } + for _, entry := range e.debugInfo.StackEntries() { + fmt.Fprintf(sb, "\n %s", entry) + }
🧹 Nitpick comments (1)
error_test.go (1)
1-758: Consider adding concurrency tests to verify thread safety.The PR objectives mention "ensuring safe concurrency without relying on shared memory," but I don't see any tests verifying concurrent access to
TrogonErrorinstances. Consider adding tests that verify:
- Concurrent reads are safe
- Mutations via
WithChangesdon't affect other goroutines- Template usage is thread-safe
Example concurrency test:
func TestTrogonErrorConcurrency(t *testing.T) { t.Run("Concurrent reads are safe", func(t *testing.T) { err := trogonerror.NewError("com.test", "TEST_ERROR", trogonerror.WithMetadataValue("key", "value", trogonerror.VisibilityPublic)) var wg sync.WaitGroup for i := 0; i < 100; i++ { wg.Add(1) go func() { defer wg.Done() _ = err.Domain() _ = err.Reason() _ = err.Metadata() _ = err.Error() }() } wg.Wait() }) t.Run("Concurrent mutations are isolated", func(t *testing.T) { original := trogonerror.NewError("com.test", "TEST_ERROR") var wg sync.WaitGroup errors := make([]*trogonerror.TrogonError, 10) for i := 0; i < 10; i++ { wg.Add(1) idx := i go func() { defer wg.Done() errors[idx] = original.WithChanges( trogonerror.WithChangeID(fmt.Sprintf("id-%d", idx))) }() } wg.Wait() // Verify each mutation is independent for i := 0; i < 10; i++ { assert.Equal(t, fmt.Sprintf("id-%d", i), errors[i].ID()) } assert.Empty(t, original.ID()) // Original unchanged }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.github/dependabot.yml(1 hunks).github/workflows/ci.yml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)doc.go(1 hunks)error.go(1 hunks)error_format_test.go(1 hunks)error_test.go(1 hunks)example_test.go(1 hunks)go.mod(1 hunks)template_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- LICENSE
- go.mod
- .github/dependabot.yml
- .github/workflows/ci.yml
- doc.go
- README.md
- template_test.go
- error_format_test.go
- example_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ldemailly
PR: TrogonStack/trogonerror#1
File: go.mod:3-3
Timestamp: 2025-07-02T22:05:52.952Z
Learning: For Go libraries, it's better to use a slightly older stable Go version (like 1.23 instead of the latest 1.24) to ensure broader compatibility with downstream users who may not have upgraded to the newest Go version yet.
🔇 Additional comments (1)
error.go (1)
1-11: Update Go version in go.mod to 1.23 for broader compatibilityThe
go.modfile currently specifies Go 1.24.2, but to ensure downstream users on slightly older toolchains can consume this library without issues, please downgrade to Go 1.23.• go.mod (line starting with
go): change- go 1.24.2 + go 1.23⛔ Skipped due to learnings
Learnt from: ldemailly PR: TrogonStack/trogonerror#1 File: go.mod:3-3 Timestamp: 2025-07-02T22:05:52.952Z Learning: For Go libraries, it's better to use a slightly older stable Go version (like 1.23 instead of the latest 1.24) to ensure broader compatibility with downstream users who may not have upgraded to the newest Go version yet.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.github/dependabot.yml(1 hunks).github/workflows/ci.yml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)Taskfile.yml(1 hunks)doc.go(1 hunks)error.go(1 hunks)error_format_test.go(1 hunks)error_test.go(1 hunks)example_test.go(1 hunks)go.mod(1 hunks)template_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- LICENSE
- doc.go
- .github/dependabot.yml
- go.mod
- .github/workflows/ci.yml
- error_format_test.go
- README.md
- example_test.go
- template_test.go
- error.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ldemailly
PR: TrogonStack/trogonerror#1
File: go.mod:3-3
Timestamp: 2025-07-02T22:05:52.952Z
Learning: For Go libraries, it's better to use a slightly older stable Go version (like 1.23 instead of the latest 1.24) to ensure broader compatibility with downstream users who may not have upgraded to the newest Go version yet.
🧬 Code Graph Analysis (1)
error_test.go (1)
error.go (48)
NewError(325-342)WithCode(345-349)Unknown(21-21)SpecVersion(14-14)Code(17-17)VisibilityInternal(42-42)Visibility(39-39)WithMetadataValue(366-370)VisibilityPublic(44-44)Metadata(65-65)MetadataValue(59-62)VisibilityPrivate(43-43)WithMetadata(359-363)WithSubject(380-384)WithID(387-391)WithTime(394-398)WithSourceID(401-405)WithHelpLink(415-425)Help(54-56)WithDebugDetail(441-449)DebugInfo(68-71)WithStackTrace(436-438)Internal(32-32)WithStackTraceDepth(452-463)WithChangeID(620-624)WithChangeSourceID(634-638)WithChangeMetadataValue(610-617)WithChangeMetadata(602-607)WithChangeTime(627-631)WithChangeHelpLink(641-651)WithChangeRetryInfoDuration(654-660)WithChangeRetryTime(663-669)RetryInfo(81-84)WithChangeLocalizedMessage(672-679)LocalizedMessage(74-77)WithCause(522-526)WithHelp(408-412)WithMessage(352-356)NotFound(24-24)TrogonError(87-105)WithWrap(536-540)WithErrorMessage(529-533)NewErrorTemplate(780-794)TemplateWithHelp(815-819)TemplateWithCode(797-801)TemplateWithMessage(803-807)TemplateWithVisibility(809-813)TemplateWithHelpLink(821-831)
🔇 Additional comments (1)
error_test.go (1)
1-755: Excellent comprehensive test coverage!This test file demonstrates thorough testing practices with:
- Well-structured subtests using
t.Run()- Comprehensive edge case coverage
- Proper table-driven tests for HTTP codes and error messages
- Good coverage of error wrapping and mutation operations
- Clear test naming that matches the actual test content
The tests effectively validate all aspects of the
trogonerrorpackage implementation.
055faf7 to
20dc76d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
error_test.go (1)
226-232: Same inefficient metadata building pattern.
🧹 Nitpick comments (2)
error_test.go (2)
39-56: Consider a more direct approach for building metadata.The current pattern creates a temporary error object just to build metadata, which seems inefficient. If the API allows, consider creating
MetadataValueobjects directly.If the API provides a constructor for
MetadataValue, you could simplify this:- metadata := make(map[string]trogonerror.MetadataValue) - - baseErr := trogonerror.NewError("temp", "TEMP") - trogonerror.WithMetadataValue("key1", "value1", trogonerror.VisibilityPublic)(baseErr) - trogonerror.WithMetadataValue("key2", "value2", trogonerror.VisibilityPrivate)(baseErr) - - for k, v := range baseErr.Metadata() { - metadata[k] = v - } + metadata := map[string]trogonerror.MetadataValue{ + "key1": trogonerror.NewMetadataValue("value1", trogonerror.VisibilityPublic), + "key2": trogonerror.NewMetadataValue("value2", trogonerror.VisibilityPrivate), + }
457-461: Remove redundant assertion.Line 461 duplicates the assertion from line 457.
assert.Equal(t, 404, code.HttpStatusCode()) assert.Equal(t, "NOT_FOUND", code.String()) assert.Equal(t, "resource not found", code.Message()) assert.NotEmpty(t, code.String()) - assert.Equal(t, code.HttpStatusCode(), 404)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.github/dependabot.yml(1 hunks).github/workflows/ci.yml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)Taskfile.yml(1 hunks)doc.go(1 hunks)error.go(1 hunks)error_format_test.go(1 hunks)error_test.go(1 hunks)example_test.go(1 hunks)go.mod(1 hunks)template_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- LICENSE
- go.mod
- .github/workflows/ci.yml
- .github/dependabot.yml
- README.md
- Taskfile.yml
- error_format_test.go
- template_test.go
- doc.go
- example_test.go
- error.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ldemailly
PR: TrogonStack/trogonerror#1
File: go.mod:3-3
Timestamp: 2025-07-02T22:05:52.952Z
Learning: For Go libraries, it's better to use a slightly older stable Go version (like 1.23 instead of the latest 1.24) to ensure broader compatibility with downstream users who may not have upgraded to the newest Go version yet.
🔇 Additional comments (2)
error_test.go (2)
14-26: LGTM!The test correctly verifies the default values for basic error creation.
679-693: Verify the intended behavior for zero and negative stack trace depths.The tests expect non-empty stack traces when depth is 0 or negative. This might be counterintuitive as depth 0 could mean "no frames". Please verify if this is the intended behavior.
If the current behavior is intentional (e.g., 0 or negative means "unlimited"), consider adding a comment to clarify this:
t.Run("WithStackTraceDepth with zero depth", func(t *testing.T) { + // Zero depth is treated as unlimited depth err := trogonerror.NewError("com.test", "TEST_ERROR", trogonerror.WithStackTraceDepth(0))
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
refactor: update function signatures to use 'any' instead of 'interface{}' for improved type flexibility
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Context
ADR: straw-hat-team/adr#30
Goals