Centralize errors formatting#72
Conversation
|
@codex[agent] review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Oleg-Melnik
left a comment
Oleg-Melnik
left a comment
There was a problem hiding this comment.
@Vladyslav-Kuksiuk LGTM with minor comment to address.
| } | ||
|
|
||
| // flattenedErrors returns the leaf errors from a joined error. | ||
| func flattenedErrors(err error) []error { |
There was a problem hiding this comment.
Why is this function name lowercase, while the function above is uppercase?
There was a problem hiding this comment.
@Oleg-Melnik In GoLang, names that start with an uppercase letter are exported, meaning they can be accessed from other packages. Names that start with a lowercase letter are unexported and can be used only within the same package. This convention provides simple visibility control without separate public or private keywords.
It's a very strange decision, but those are the rules of the language.
This is a docs.
There was a problem hiding this comment.
In Go, names starting with an uppercase letter are public and accessible from other packages. Names starting with a lowercase letter are private to their package.
There was a problem hiding this comment.
Pull request overview
This PR centralizes user-facing joined-error formatting into a shared helper in logging/error.go, removing duplication between main.go and the panic-logging path. It also refactors logging tests to use the repository’s Ginkgo/Gomega Describe/It style.
Changes:
- Extracted joined/nested error flattening + bullet-list formatting into
logging.FormatError. - Rewired CLI error exit logging and panic formatting to use
logging.FormatError. - Converted
logging/logger_test.goto a Ginkgo/Gomega spec suite and added new specs for error formatting.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| main.go | Replaces local error-formatting helpers with logging.FormatError for exit logging. |
| logging/logger.go | Simplifies panic message formatting by delegating to FormatError. |
| logging/logger_test.go | Migrates file URL tests to Ginkgo/Gomega suite style. |
| logging/error.go | New shared implementation for formatting single vs joined/nested errors. |
| logging/error_test.go | Adds specs intended to cover single/nested/joined error formatting behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import ( | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| "embed-code/embed-code-go/logging" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| var _ = Describe("Error formatting", func() { |
There was a problem hiding this comment.
That is not true. All eight tests - four from logger_test and four from error_test - run successfully.
| builder.WriteString(message) | ||
| builder.WriteString(":") | ||
| for _, nestedErr := range errs { | ||
| builder.WriteString("\n- ") |
There was a problem hiding this comment.
I think it is better to use platform-specific line separator.
I didn't find a method for it in Go (but try do it by yourself, may be I'm just blind).
If not let's create a util function. Smth like:
lineSeparator := "\n"
if runtime.GOOS == "windows" {
lineSeparator = "\r\n"
}There was a problem hiding this comment.
Go conventionally uses LF for formatted output on every platform. The fmt.Fprintln also emits \n.
| builder.WriteString(message) | ||
| builder.WriteString(":") | ||
| for _, nestedErr := range errs { | ||
| builder.WriteString("\n- ") |
There was a problem hiding this comment.
Also I think it will be prettier with tabulation.
builder.WriteString(lineSeparator)
builder.WriteString("\t- ")
builder.WriteString(nestedErr.Error())| "\n- first failure" + | ||
| "\n- second failure" + | ||
| "\n- third failure"), | ||
| ) |
There was a problem hiding this comment.
Equal("operation failed:\n" +
"- first failure\n" +
"- second failure\n" +
"- third failure"),
This PR extracts the error formatting to own
logging/error.gowhich removes code duplication.Additionally changed the logging tests format to match the project
Describe-Itformat.Resolves this issue.