-
Notifications
You must be signed in to change notification settings - Fork 373
fix(errors): replace fmt.Errorf("%s") with errors.New and preserve error chains in pkg/workflow + pkg/parser #29197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -65,7 +65,7 @@ func FormatImportCycleError(err *ImportCycleError) error { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| messageBuilder.WriteString("2. Remove one of the imports to break the cycle\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| messageBuilder.WriteString("3. Consider restructuring your workflow imports to avoid circular dependencies\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Errorf("%s", messageBuilder.String()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &FormattedParserError{formatted: messageBuilder.String(), cause: err} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // formatImportCycleErrorRegressionCheck verifies the wrapping contract relied on by callers: | |
| // the formatted message stays stable and the original *ImportCycleError remains discoverable | |
| // via errors.As through the returned *FormattedParserError. | |
| func formatImportCycleErrorRegressionCheck() error { | |
| cycleErr := &ImportCycleError{ | |
| Chain: []string{"a.md", "b.md", "a.md"}, | |
| WorkflowFile: "workflow.md", | |
| } | |
| formattedErr := FormatImportCycleError(cycleErr) | |
| expectedMessage := "" + | |
| "Import cycle detected\n\n" + | |
| "The following import chain creates a circular dependency:\n\n" + | |
| "a.md (starting point)\n" + | |
| " ↳ imports b.md\n" + | |
| " ↳ a.md ⚠️ cycles back to a.md\n" + | |
| "\nTo fix this issue:\n" + | |
| "1. Review the import dependencies in the files listed above\n" + | |
| "2. Remove one of the imports to break the cycle\n" + | |
| "3. Consider restructuring your workflow imports to avoid circular dependencies\n" | |
| if formattedErr.Error() != expectedMessage { | |
| return fmt.Errorf("unexpected formatted import cycle error message") | |
| } | |
| var cycleErrOut *ImportCycleError | |
| if !errors.As(formattedErr, &cycleErrOut) { | |
| return fmt.Errorf("expected errors.As to recover *ImportCycleError") | |
| } | |
| if cycleErrOut != cycleErr { | |
| return fmt.Errorf("expected recovered *ImportCycleError to match original") | |
| } | |
| return nil | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New behavior returns a *FormattedParserError that unwraps the underlying YAML unmarshal error, but there isn’t a test covering that the cause is actually reachable via errors.As/errors.Unwrap. Since the PR’s goal is to preserve error chains, please add a regression test that calls ExtractFrontmatterFromContent with invalid YAML and asserts the returned error unwraps to the underlying goccy/go-yaml error type (e.g., yaml.SyntaxError / yaml.TypeError), while still keeping the formatted message prefix.