feat: create formatters to simplify the request output#20
Conversation
WalkthroughThis pull request consolidates and streamlines several aspects of the application’s behavior. In the command runner ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Run as cmd/run.go
participant NP as printer.NewPrinter
participant P as Printer
participant F as Formatter
participant O as os.Stdout
User->>Run: Execute command
Run->>NP: Instantiate Printer with config
NP->>P: Initialize Printer (set formatter via NewFormatter)
Run->>P: Call Print(result)
P->>F: Format request/response details
P->>O: Write formatted output
sequenceDiagram
participant C as Client
participant R as Request
participant RES as Result
C->>R: Receive HTTP request
C->>RES: Construct Result using composite literal<br/>(Response, RequestBody, Metadata)
RES-->>C: Return Result
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Definitions (2)pkg/printer/printer_test.go (3)
pkg/printer/formatter_test.go (3)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (19)
pkg/result/result.go (3)
28-34: Potential duplication between status and request lines.
Consider pulling out common fields into a shared type if these line representations continue to grow in features. This can promote DRY principles.
36-45: Lack of error logging on JSON unmarshal.
When JSON decoding fails, the code currently returnsnilwithout surfacing the issue. Consider logging or otherwise indicating invalid request body content.if err := json.Unmarshal(body, &data); err != nil { + // Log the error or handle it more gracefully return nil }
52-63: Silent JSON parse failures for response.
As withRequestJson, returningnilon parse failure can mask issues. Consider a small logging message or error callback to diagnose unexpected response formats.if err := json.Unmarshal(r.ResponseBody, &data); err != nil { + // Possibly log the error or propagate it return nil }pkg/printer/printer.go (2)
26-27: Use struct fields or constructor arguments in place of global vars.
destinationandisTerminalare currently global. If you want multiple printer instances or advanced usage, consider parameterizing these values.-var destination io.Writer = os.Stdout -var isTerminal = term.IsTerminal(int(os.Stdout.Fd())) +func NewPrinter(config PrinterConfig, out io.Writer, isTerm bool) Printer { + formatter := NewFormatter(isTerm) + return Printer{ + config: config, + formatter: formatter, + destination: out, + } +}
38-79: Single method for printing request/response details.
Consolidating headers and body printing intofunc (p *Printer) Print(res *result.Result) { sections := []string{} + // Example: separate method for request header printing: + if p.config.PrintRequestHeaders { + sections = append(sections, p.printReqHeaders(res)) + } + fmt.Fprint(destination, strings.Join(sections, "\n")) }pkg/printer/formatter_test.go (3)
77-90: Fix cuddling assignments issue according to golangci-lintAccording to the static analysis tool, there's a style issue related to code structure. Assignments should be properly separated from if statements.
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var formatter interface { StatusLine(result.StatusLine) string } + if tt.useColor { formatter = ColorFormatter{} } else { formatter = PlainFormatter{} } + got := formatter.StatusLine(tt.line) assert.Equal(t, tt.want, got) })🧰 Tools
🪛 GitHub Check: golangci
[failure] 87-87:
assignments should only be cuddled with other assignments (wsl)
[failure] 82-82:
if statements should only be cuddled with assignments (wsl)🪛 golangci-lint (1.64.8)
87-87: assignments should only be cuddled with other assignments
(wsl)
82-82: if statements should only be cuddled with assignments
(wsl)
🪛 GitHub Actions: .github/workflows/ci.yml
[error] 82-82: if statements should only be cuddled with assignments (wsl)
173-186: Fix cuddling assignments issue according to golangci-lintSimilar to the previous issue, the assignments should be properly separated from if statements.
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var formatter interface { RequestLine(result.RequestLine) string } + if tt.useColor { formatter = ColorFormatter{} } else { formatter = PlainFormatter{} } + got := formatter.RequestLine(tt.line) assert.Equal(t, tt.want, got) })🧰 Tools
🪛 GitHub Check: golangci
[failure] 183-183:
assignments should only be cuddled with other assignments (wsl)
[failure] 178-178:
if statements should only be cuddled with assignments (wsl)🪛 golangci-lint (1.64.8)
178-178: if statements should only be cuddled with assignments
(wsl)
183-183: assignments should only be cuddled with other assignments
(wsl)
276-289: Fix cuddling assignments issue according to golangci-lintAnother instance of the same style issue. The assignments should be properly separated from if statements.
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var formatter interface { Headers(http.Header) string } + if tt.useColor { formatter = ColorFormatter{} } else { formatter = PlainFormatter{} } + got := formatter.Headers(tt.headers) assert.Equal(t, tt.want, got) })🧰 Tools
🪛 GitHub Check: golangci
[failure] 286-286:
assignments should only be cuddled with other assignments (wsl)
[failure] 281-281:
if statements should only be cuddled with assignments (wsl)🪛 golangci-lint (1.64.8)
281-281: if statements should only be cuddled with assignments
(wsl)
286-286: assignments should only be cuddled with other assignments
(wsl)
pkg/printer/colors.go (4)
1-6: Good package and type definition!The Color type alias provides clear semantic meaning. Consider adding a documentation comment explaining the purpose of this type to improve code clarity.
+// Color represents an ANSI terminal color code as a string type Color = string
7-15: Add documentation for color constantsThe color constants are well-defined and follow a consistent naming pattern. Consider adding a comment block to explain these color codes and their purpose.
+// ANSI color codes for terminal output const ( ColorReset Color = "\033[0m" ColorRed Color = "\033[31m" ColorGreen Color = "\033[32m" ColorYellow Color = "\033[33m" ColorBlue Color = "\033[34m" ColorMagenta Color = "\033[35m" ColorCyan Color = "\033[36m" )
17-19: Good implementation of the Colorize functionThe function properly handles wrapping a string with color codes. However, for better efficiency, consider using string concatenation instead of fmt.Sprintf.
func Colorize(s string, c Color) string { - return fmt.Sprintf("%v%s%v", c, s, ColorReset) + return c + s + ColorReset }
21-43: Refactor color functions to use ColorizeThe color-specific functions use direct string concatenation while the Colorize function uses fmt.Sprintf. For better consistency and to reduce code duplication, consider refactoring these functions to leverage the Colorize function.
func Red(s string) string { - return ColorRed + s + ColorReset + return Colorize(s, ColorRed) } func Green(s string) string { - return ColorGreen + s + ColorReset + return Colorize(s, ColorGreen) } func Yellow(s string) string { - return ColorYellow + s + ColorReset + return Colorize(s, ColorYellow) } func Blue(s string) string { - return ColorBlue + s + ColorReset + return Colorize(s, ColorBlue) } func Magenta(s string) string { - return ColorMagenta + s + ColorReset + return Colorize(s, ColorMagenta) } func Cyan(s string) string { - return ColorCyan + s + ColorReset + return Colorize(s, ColorCyan) }pkg/printer/formatter.go (7)
13-18: Add documentation to the Formatter interfaceThe interface is well-defined, but would benefit from documentation comments explaining the purpose of each method.
+// Formatter provides methods to format different components of HTTP requests and responses type Formatter interface { + // StatusLine formats the status line of an HTTP response StatusLine(statusLine result.StatusLine) string + // RequestLine formats the request line of an HTTP request RequestLine(requestLine result.RequestLine) string + // Headers formats HTTP headers Headers(headers http.Header) string + // Json formats a JSON object Json(map[string]interface{}) string }
20-26: Add documentation to the NewFormatter functionConsider adding a documentation comment explaining the purpose of the function and its parameter.
+// NewFormatter creates a new formatter based on terminal capabilities +// Returns ColorFormatter for terminal output, PlainFormatter otherwise func NewFormatter(isTerminal bool) Formatter { if isTerminal { return ColorFormatter{} } return PlainFormatter{} }
28-29: Add documentation to formatter typesConsider adding documentation comments explaining the purpose of each formatter type.
+// ColorFormatter provides colored output formatting for terminal display type ColorFormatter struct{} +// PlainFormatter provides uncolored output formatting for non-terminal display type PlainFormatter struct{}
63-78: Use strings.Builder for more efficient header formattingThe current implementation uses += to build the string, which creates a new string allocation for each header. Using strings.Builder would be more efficient, especially for requests with many headers.
func formatHeaders(headers http.Header, colorized bool) string { - s := "" + var sb strings.Builder fstring := "%s: %s\n" for key, value := range headers { vs := strings.Join(value, ", ") if colorized { - s += fmt.Sprintf(fstring, Cyan(key), vs) + fmt.Fprintf(&sb, fstring, Cyan(key), vs) } else { - s += fmt.Sprintf(fstring, key, vs) + fmt.Fprintf(&sb, fstring, key, vs) } } - return s + return sb.String() }
80-113: Improve status code detection and document color logicInstead of using string prefix checks, consider parsing the status code for clearer category checking. Also, document the color scheme to improve maintainability.
func formatStatusLine(line result.StatusLine, colorized bool) string { fstring := "%v %v [%v]\n" if !colorized { return fmt.Sprintf(fstring, line.Protocol, line.Status, line.Time) } var timeColor, statusColor Color + // Color status codes by category + // 1xx, 2xx: Green + // 3xx: Cyan + // 4xx, 5xx: Red switch { case strings.HasPrefix(line.Status, "2"), strings.HasPrefix(line.Status, "1"): statusColor = ColorGreen case strings.HasPrefix(line.Status, "3"): statusColor = ColorCyan default: statusColor = ColorRed } + // Color response time based on duration + // < 500ms: Default + // 500ms-1s: Yellow + // > 1s: Red switch { case line.Time < 500*time.Millisecond: timeColor = ColorReset case line.Time < 1000*time.Millisecond: timeColor = ColorYellow default: timeColor = ColorRed }
115-136: Document HTTP method color logic and handle case insensitivityAdd comments explaining the color scheme for HTTP methods and consider making the method matching case-insensitive for better robustness.
func formatRequestLine(line result.RequestLine, colorized bool) string { fstring := "%v %v %v\n" if !colorized { return fmt.Sprintf(fstring, line.Method, line.Url, line.Protocol) } var methodColor Color + // Color HTTP methods based on their semantics + // GET, HEAD: Green (safe methods) + // POST, PUT, PATCH: Yellow (resource creation/modification) + // DELETE: Red (resource removal) + // Others: Magenta switch line.Method { - case "GET", "HEAD": + case "GET", "HEAD", "get", "head": methodColor = ColorGreen - case "POST", "PUT", "PATCH": + case "POST", "PUT", "PATCH", "post", "put", "patch": methodColor = ColorYellow - case "DELETE": + case "DELETE", "delete": methodColor = ColorRed default: methodColor = ColorMagenta }Alternatively, use strings.ToUpper() for a more standardized approach:
func formatRequestLine(line result.RequestLine, colorized bool) string { fstring := "%v %v %v\n" if !colorized { return fmt.Sprintf(fstring, line.Method, line.Url, line.Protocol) } var methodColor Color + method := strings.ToUpper(line.Method) switch method { case "GET", "HEAD": methodColor = ColorGreen case "POST", "PUT", "PATCH": methodColor = ColorYellow case "DELETE": methodColor = ColorRed default: methodColor = ColorMagenta } return fmt.Sprintf(fstring, Colorize(line.Method, methodColor), line.Url, line.Protocol) }
138-144: Improve JSON formatting error handlingThe current implementation returns a generic error message when JSON parsing fails. Consider providing more detailed error information.
func formatJson(j map[string]interface{}) string { if prettyJson, err := json.MarshalIndent(j, "", " "); err != nil { - return "unable to parse json" + return fmt.Sprintf("unable to parse json: %v", err) } else { return string(prettyJson) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
cmd/run.go(1 hunks)go.mod(1 hunks)pkg/client/client.go(1 hunks)pkg/expectation/expectation.go(0 hunks)pkg/expectation/expectation_test.go(0 hunks)pkg/printer/colors.go(1 hunks)pkg/printer/constants.go(0 hunks)pkg/printer/formatter.go(1 hunks)pkg/printer/formatter_test.go(1 hunks)pkg/printer/printer.go(2 hunks)pkg/printer/printer_test.go(1 hunks)pkg/result/result.go(1 hunks)pkg/result/result_test.go(0 hunks)
💤 Files with no reviewable changes (4)
- pkg/printer/constants.go
- pkg/expectation/expectation_test.go
- pkg/expectation/expectation.go
- pkg/result/result_test.go
🧰 Additional context used
🧬 Code Definitions (7)
pkg/printer/printer.go (2)
pkg/printer/formatter.go (2)
Formatter(13-18)NewFormatter(20-26)pkg/result/result.go (3)
Result(9-14)RequestLine(75-79)StatusLine(69-73)
pkg/client/client.go (1)
pkg/result/result.go (2)
Result(9-14)Metadata(16-18)
pkg/printer/formatter_test.go (3)
pkg/result/result.go (2)
StatusLine(69-73)RequestLine(75-79)pkg/printer/colors.go (8)
Colorize(17-19)ColorGreen(10-10)ColorReset(8-8)ColorCyan(14-14)ColorYellow(11-11)ColorRed(9-9)ColorMagenta(13-13)Cyan(41-43)pkg/printer/formatter.go (2)
ColorFormatter(28-28)PlainFormatter(29-29)
cmd/run.go (1)
pkg/printer/printer.go (2)
NewPrinter(29-36)PrinterConfig(14-19)
pkg/printer/formatter.go (2)
pkg/result/result.go (2)
StatusLine(69-73)RequestLine(75-79)pkg/printer/colors.go (9)
Cyan(41-43)Color(5-5)ColorGreen(10-10)ColorCyan(14-14)ColorRed(9-9)ColorReset(8-8)ColorYellow(11-11)Colorize(17-19)ColorMagenta(13-13)
pkg/printer/printer_test.go (3)
pkg/printer/printer.go (2)
NewPrinter(29-36)PrinterConfig(14-19)pkg/printer/formatter.go (2)
ColorFormatter(28-28)PlainFormatter(29-29)pkg/result/result.go (2)
Result(9-14)Metadata(16-18)
pkg/result/result.go (1)
pkg/request/request.go (1)
Request(21-26)
🪛 GitHub Check: golangci
pkg/printer/formatter_test.go
[failure] 286-286:
assignments should only be cuddled with other assignments (wsl)
[failure] 281-281:
if statements should only be cuddled with assignments (wsl)
[failure] 183-183:
assignments should only be cuddled with other assignments (wsl)
[failure] 178-178:
if statements should only be cuddled with assignments (wsl)
[failure] 87-87:
assignments should only be cuddled with other assignments (wsl)
[failure] 82-82:
if statements should only be cuddled with assignments (wsl)
🪛 golangci-lint (1.64.8)
pkg/printer/formatter_test.go
87-87: assignments should only be cuddled with other assignments
(wsl)
178-178: if statements should only be cuddled with assignments
(wsl)
183-183: assignments should only be cuddled with other assignments
(wsl)
281-281: if statements should only be cuddled with assignments
(wsl)
286-286: assignments should only be cuddled with other assignments
(wsl)
82-82: if statements should only be cuddled with assignments
(wsl)
🪛 GitHub Actions: .github/workflows/ci.yml
pkg/printer/formatter_test.go
[error] 82-82: if statements should only be cuddled with assignments (wsl)
🔇 Additional comments (18)
pkg/result/result.go (5)
10-14: Consider potential memory usage and data consistency.
Storing the entire*http.Responsein this struct can lead to increased memory usage or possible data inconsistencies if the response body remains open or un-consumed. Please ensure the response body is properly closed elsewhere in the codebase to avoid resource leaks.Would you like me to generate a script to locate any usage patterns that leave the response body open or unhandled?
20-26: Pointer return pattern considered valid.
Returning a pointer to a transientStatusLinestruct is fine, but ensure that all consumers of this method consistently handle the struct’s lifetime.
47-50: Straightforward text retrieval.
TheRequestText()logic is concise, returning the raw request body as a string with no concerns raised.
65-67: Simple text output for response body.
TheText()method is a direct conversion from bytes to string, which is perfectly acceptable.
69-79: StatusLine and RequestLine structs look consistent.
These struct definitions align well with the rest of the changes. Having distinct types for request and status lines helps maintain clarity in formatting.go.mod (2)
13-13: Explicit direct dependency on cobra.
Listinggithub.com/spf13/cobra v1.8.0as a direct requirement clarifies usage and improves overall dependency tracking.
3-5:Details
✅ Verification successful
Confirm new Go versions in the pipeline.
Switching togo 1.23.0and specifyingtoolchain go1.24.0can be beneficial. Ensure your CI/CD environment supports these versions without conflicts.
🏁 Script executed:
#!/bin/bash # Check the installed Go version in the current environment go versionLength of output: 44
CI/CD Go Version Verified
The pipeline is currently running Go version 1.24.1, which is compatible with your declared toolchain of go1.24.0—minor patch differences aside. The module still specifies go 1.23.0, which may be intentional if you're gradually upgrading or targeting specific language features. If consistency across directives is desired, consider updating either thegodirective or the toolchain version to match the installed Go release. Otherwise, the configuration appears supported by your CI/CD environment.pkg/printer/printer.go (3)
5-11: Imports reorganized for advanced I/O and terminal detection.
Addingio,os,golang.org/x/term, and referencing theresultpackage directly are consistent with the new single-point printing approach.
22-23: Configuration and formatter separation.
DefiningPrinterConfigandFormatteras standalone fields fosters clear separation of concerns, simplifying future enhancements to each.
30-36: Dynamic formatter initialization.
Creating a formatter based onisTerminalis a clean way to transparently switch between color and plain output.pkg/printer/formatter_test.go (4)
13-91: Test_formatStatusLine is well structured and comprehensive.The test covers various scenarios including non-colorized and colorized status lines with different response times and status codes. The test is well structured using a table-driven approach.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 87-87:
assignments should only be cuddled with other assignments (wsl)
[failure] 82-82:
if statements should only be cuddled with assignments (wsl)🪛 golangci-lint (1.64.8)
87-87: assignments should only be cuddled with other assignments
(wsl)
82-82: if statements should only be cuddled with assignments
(wsl)
🪛 GitHub Actions: .github/workflows/ci.yml
[error] 82-82: if statements should only be cuddled with assignments (wsl)
93-187: Test_formatRequestLine is well structured and validates different HTTP methods.The test properly verifies both colorized and non-colorized representation of various HTTP methods (GET, POST, DELETE, OPTIONS) with appropriate colors.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 183-183:
assignments should only be cuddled with other assignments (wsl)
[failure] 178-178:
if statements should only be cuddled with assignments (wsl)🪛 golangci-lint (1.64.8)
178-178: if statements should only be cuddled with assignments
(wsl)
183-183: assignments should only be cuddled with other assignments
(wsl)
189-234: Test_formatJson includes good error handling for unmarshalable values.The test correctly handles various JSON serialization scenarios including simple objects, nested objects, empty objects, and unmarshalable values (channel). The error case is particularly valuable for robust testing.
236-290: Test_formatHeaders effectively tests different header scenarios.The test covers various scenarios including no headers, single header (colorized and non-colorized), and multiple headers. This comprehensive approach ensures the formatter handles all expected cases.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 286-286:
assignments should only be cuddled with other assignments (wsl)
[failure] 281-281:
if statements should only be cuddled with assignments (wsl)🪛 golangci-lint (1.64.8)
281-281: if statements should only be cuddled with assignments
(wsl)
286-286: assignments should only be cuddled with other assignments
(wsl)
pkg/client/client.go (1)
60-67: Simplified result construction with direct struct initialization.The code has been refactored to use a direct struct initialization approach, which is cleaner and more idiomatic Go. The previous approach likely used a helper function with error handling that has been determined to be unnecessary.
cmd/run.go (1)
65-66: Simplified output printing with unified printer instance.The code has been greatly simplified by replacing multiple explicit print operations with a unified printer instance. This approach is more maintainable and follows the Single Responsibility Principle by delegating the printing logic to a dedicated component.
pkg/printer/printer_test.go (2)
14-47: Well-structured test for NewPrinter with clear expectations.This test properly validates that the correct formatter type is selected based on terminal status. The test setup also correctly preserves and restores global state using
defer, which is important for test isolation.
49-125: Comprehensive Print method test with robust setup and validation.The test for the Print method is well structured:
- It properly captures output using a buffer
- It correctly restores global state using defer
- It tests multiple configurations with clear expectations
- It verifies output content in a flexible way using string contains checks
This is an effective approach for testing complex output formatting functions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/printer/formatter_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pkg/printer/formatter_test.go (3)
pkg/result/result.go (2)
StatusLine(69-73)RequestLine(75-79)pkg/printer/colors.go (8)
Colorize(17-19)ColorGreen(10-10)ColorReset(8-8)ColorCyan(14-14)ColorYellow(11-11)ColorRed(9-9)ColorMagenta(13-13)Cyan(41-43)pkg/printer/formatter.go (2)
ColorFormatter(28-28)PlainFormatter(29-29)
🪛 GitHub Actions: .github/workflows/ci.yml
pkg/printer/formatter_test.go
[error] 293-293: Test failed: expected equal; got: Content-Type: application/json
X-Custom: value1, value2
; want: X-Custom: value1, value2
Content-Type: application/json
🔇 Additional comments (3)
pkg/printer/formatter_test.go (3)
13-93: Well-structured test for HTTP status line formatting!The test covers various status codes and response times, verifying both colorized and non-colorized outputs. This table-driven approach is clean and comprehensive.
95-191: Great test coverage for HTTP request line formattingThis test effectively covers different HTTP methods with appropriate color coding for each method type. The testing structure is consistent with the previous test.
193-238: JSON formatting test looks goodThe test covers essential JSON formatting scenarios including simple, nested, empty objects, and error handling for unmarshalable values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/printer/formatter.go (5)
21-27: Consider making the formatter option more descriptive than a boolean.Relying on a single boolean (
isTerminal) might be less expressive than using a dedicated type or enum, e.g.FormatterTypeColorvs.FormatterTypePlain. Such an approach makes the code more self-documenting and open for future extensibility (e.g., more than two formatters).
64-86: Optimize repeated string concatenation.Building large strings in a loop can benefit from using a
strings.Builder. This can reduce allocations and improve performance, especially for responses with numerous headers.func formatHeaders(headers http.Header, colorized bool) string { - s := "" fstring := "%s: %s\n" keys := make([]string, 0, len(headers)) for k := range headers { keys = append(keys, k) } sort.Strings(keys) - for _, key := range keys { - value := strings.Join(headers[key], ", ") - if colorized { - s += fmt.Sprintf(fstring, Cyan(key), value) - } else { - s += fmt.Sprintf(fstring, key, value) - } - } - return s + var sb strings.Builder + for _, key := range keys { + value := strings.Join(headers[key], ", ") + if colorized { + sb.WriteString(fmt.Sprintf(fstring, Cyan(key), value)) + } else { + sb.WriteString(fmt.Sprintf(fstring, key, value)) + } + } + return sb.String() }
88-121: Evaluate status parsing to handle numeric HTTP codes more precisely.Currently, we’re coloring based on string prefixes (e.g.,
"2"for 2xx codes,"3"for 3xx). If the status field might include unconventional patterns, consider parsing the numeric status code for more robust handling.
123-144: Handle case-insensitive HTTP methods.If HTTP methods can come in various cases (e.g., “get” instead of “GET”), convert them to uppercase or a canonical form before applying color logic. This ensures consistent colorization for all valid HTTP methods.
func formatRequestLine(line result.RequestLine, colorized bool) string { ... - switch line.Method { + method := strings.ToUpper(line.Method) + switch method { ... } ... }
146-152: Enhance error handling for unparseable JSON.Currently, an error results in a generic “unable to parse json” message. Consider returning the error details or a structured error to help debug JSON issues without losing important context.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/printer/formatter.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pkg/printer/formatter.go (2)
pkg/result/result.go (2)
StatusLine(69-73)RequestLine(75-79)pkg/printer/colors.go (9)
Cyan(41-43)Color(5-5)ColorGreen(10-10)ColorCyan(14-14)ColorRed(9-9)ColorReset(8-8)ColorYellow(11-11)Colorize(17-19)ColorMagenta(13-13)
🔇 Additional comments (2)
pkg/printer/formatter.go (2)
14-19: Well-structured interface definition.This interface elegantly captures all the required methods for HTTP output formatting (status lines, request lines, headers, and JSON), promoting consistency across different implementations.
32-38: Clear separation of color vs. plain header formatting.Both implementations correctly delegate to
formatHeaders, ensuring minimal code duplication. This is clean and consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
pkg/result/result.go (3)
34-39: ValidateResponsebefore accessing its fields.
r.Response.Protoandr.Response.Statusare called without checks, which can panic ifr.Responseis nil. Consider defensive coding to avoid unexpected runtime errors.func (r Result) StatusLine() *StatusLine { - return &StatusLine{ - Protocol: r.Response.Proto, - Status: r.Response.Status, - Time: r.Metadata.ElapsedTime, - } + if r.Response == nil { + return &StatusLine{ + Protocol: "", + Status: "", + Time: r.Metadata.ElapsedTime, + } + } + return &StatusLine{ + Protocol: r.Response.Proto, + Status: r.Response.Status, + Time: r.Metadata.ElapsedTime, + } }
42-48: Check for a nil request inRequestLine().
r.Response.Requestmay be nil if the response was not formed by a standard HTTP client or if an error occurred. Consider verifying thatr.Response.Requestis non-nil to prevent panics.func (r Result) RequestLine() *RequestLine { - return &RequestLine{ - Protocol: r.Response.Request.Proto, - Url: r.Response.Request.URL.String(), - Method: r.Response.Request.Method, - } + if r.Response == nil || r.Response.Request == nil { + return &RequestLine{} + } + return &RequestLine{ + Protocol: r.Response.Request.Proto, + Url: r.Response.Request.URL.String(), + Method: r.Response.Request.Method, + } }
50-61: Surface JSON unmarshal errors.
Currently, the function returnsnilon any unmarshal error, offering no feedback. Consider returning the error or storing it in theResultfor better observability and troubleshooting.func (b Body) Json() map[string]any { if len(b) == 0 { return nil } var data map[string]any if err := json.Unmarshal(b, &data); err != nil { - return nil + // Return an empty map or attach error details in some way + return map[string]any{ + "unmarshal_error": err.Error(), + } } return data }pkg/printer/printer.go (4)
26-27: Avoid using package-level globals fordestinationandisTerminal.
Storing these as global variables may affect testability and reusability. Consider using a struct field or constructor injection for improved isolation.-var destination io.Writer = os.Stdout -var isTerminal = term.IsTerminal(int(os.Stdout.Fd())) +func NewPrinter(config PrinterConfig, dest io.Writer, checkTerminal bool) Printer { + ... +}
60-67: Prevent copyingRequestby value.
req := *res.Response.Requestcopies the entire request struct. If the request is large or changes, it can be inefficient or misleading. Consider referencing the request struct directly instead of copying.-func (p *Printer) printRequestHeaders(res *result.Result) string { - req := *res.Response.Request +func (p *Printer) printRequestHeaders(res *result.Result) string { + req := res.Response.Request s := "" - s += p.formatter.RequestLine(*res.RequestLine()) - s += p.formatter.Headers(req.Header) + s += p.formatter.RequestLine(*res.RequestLine()) // if safe + s += p.formatter.Headers(req.Header) return s }
69-75: Consistent style inprintResponseHeaders.
The logic for printing needs to ensure consistent spacing, newlines, and optional color formatting. Consider standardizing withprintRequestHeadersor reusing a helper function.
77-87: Consider multi-format support for large bodies.
For bigger responses, printing everything at once might degrade performance. Think about streaming or chunking.pkg/printer/formatter.go (3)
32-38: Avoid confusion with color-coded headers.
Currently, the code color-codes header keys but not values. This is acceptable, but you might color-code both keys and values for increased readability.
56-62: Consider colorizing JSON keys.
In the color formatter,Json()currently callsformatJsonwithout color. If desired, you can parse, colorize keys and strings.
146-153: Return better error messages for JSON failures.
When pretty-printing JSON fails, it currently returns a generic string. Consider returning original JSON or error data for easier debugging.func formatJson(j map[string]any) string { if prettyJson, err := json.MarshalIndent(j, "", " "); err != nil { - return "unable to parse json" + return fmt.Sprintf("unable to parse json: %v", err) } else { return string(prettyJson) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod(1 hunks)pkg/printer/colors.go(1 hunks)pkg/printer/formatter.go(1 hunks)pkg/printer/printer.go(2 hunks)pkg/result/result.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/printer/colors.go
🧰 Additional context used
🧬 Code Definitions (2)
pkg/result/result.go (1)
pkg/request/request.go (1)
Request(21-26)
pkg/printer/formatter.go (2)
pkg/result/result.go (2)
StatusLine(20-24)RequestLine(26-30)pkg/printer/colors.go (9)
Cyan(39-41)Color(3-3)ColorGreen(8-8)ColorCyan(12-12)ColorRed(7-7)ColorReset(6-6)ColorYellow(9-9)Colorize(15-17)ColorMagenta(11-11)
🔇 Additional comments (8)
pkg/result/result.go (2)
10-14: Guard against potential nilResponsefield.
Ifr.Responseis ever nil, dereferencing it at lines 10-14 may cause a runtime panic. Ensure that callers always populater.Responseor add a nil check.Would you like to explore the codebase to confirm
Result.Responseis never nil? I can generate a script to locate allResultinstantiations and verify thatResponseis always assigned.
63-65: Looks good!
TheText()method is straightforward and appears correct, converting the raw bytes to a string.pkg/printer/printer.go (1)
22-23: Ensure thread-safety if concurrent prints can occur.
Printerstores config and a formatter. If multiple goroutines print simultaneously, concurrency issues may arise. Confirm that this code won't be called from concurrent contexts without synchronization.Would you like me to inspect usage sites of
Printerto confirm single-threaded usage?pkg/printer/formatter.go (5)
40-46: Minimal overhead inStatusLine().
This method is concise and effectively delegates toformatStatusLine. No issues found, well done!
48-54: Well-structuredRequestLine()method.
Delegation toformatRequestLineis clean and maintains single-responsibility. No concerns.
64-86: Header sorting is correct.
Sorting header keys alphabetically is good for consistency, ensuring repeated runs produce stable output.
88-122: Status line color logic is intuitive.
The status code prefix-based color scheme is straightforward, and the time-based coloring is also clear. Good job on readability.
123-145: Method-based color coding is well-chosen.
Each HTTP method is assigned a distinctive color that stands out appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
pkg/printer/printer.go (4)
26-27: Consider making the global variables configurable per instanceUsing global variables like
destinationandisTerminalcan make testing difficult and introduce unexpected side effects. Consider making these configurable perPrinterinstance or using dependency injection to improve testability and maintainability.type Printer struct { config PrinterConfig formatter Formatter + destination io.Writer + isTerminal bool } -var destination io.Writer = os.Stdout -var isTerminal = term.IsTerminal(int(os.Stdout.Fd())) func NewPrinter(config PrinterConfig) Printer { + destination := os.Stdout + isTerminal := term.IsTerminal(int(os.Stdout.Fd())) formatter := NewFormatter(isTerminal) return Printer{ config: config, formatter: formatter, + destination: destination, + isTerminal: isTerminal, } }
65-72: Consider using a string builder for better performanceUsing string concatenation (
s += ...) can be inefficient as it creates new string objects with each operation. Consider usingstrings.Builderfor better performance, especially when dealing with larger requests.func (p *Printer) printRequestHeaders(res *result.Result) string { if res.Response.Request == nil { return "No HTTP request available\n" } req := *res.Response.Request - s := "" + var b strings.Builder - s += p.formatter.RequestLine(*res.RequestLine()) - s += p.formatter.Headers(req.Header) + b.WriteString(p.formatter.RequestLine(*res.RequestLine())) + b.WriteString(p.formatter.Headers(req.Header)) - return s + return b.String() }
74-80: Consider using a string builder here tooSimilar to the previous comment, use
strings.Builderhere as well for consistent implementation and better performance.func (p *Printer) printResponseHeaders(res *result.Result) string { - s := "" + var b strings.Builder - s += p.formatter.StatusLine(*res.StatusLine()) - s += p.formatter.Headers(res.Response.Header) + b.WriteString(p.formatter.StatusLine(*res.StatusLine())) + b.WriteString(p.formatter.Headers(res.Response.Header)) - return s + return b.String() }
82-92: Handle potential nil bodyConsider adding a check for a nil body pointer to prevent potential panics.
func (p *Printer) printBody(body *result.Body) string { + if body == nil { + return "" + } if jsonBody := body.Json(); jsonBody != nil { return fmt.Sprintf("%v\n", p.formatter.Json(jsonBody)) } if textBody := body.Text(); textBody != "" { return fmt.Sprintf("%v\n", textBody) } return "" }pkg/printer/printer_test.go (3)
52-58: Consider refactoring to avoid global variable manipulationModifying global variables in tests can lead to side effects between tests if they run in parallel. Consider refactoring the
Printerto accept these values through constructor parameters, as suggested for the main implementation.func TestPrinter_Print(t *testing.T) { var buf bytes.Buffer - oldDestination := destination - destination = &buf - oldIsTerminal := isTerminal - isTerminal = false - - defer func() { destination = oldDestination }() - defer func() { isTerminal = oldIsTerminal }() // ... rest of test ... }Then modify how you create the printer:
p := NewPrinter(tc.config) // Replace with something like: p := NewPrinterWithCustom(tc.config, &buf, false)
77-109: Comprehensive test casesThe test cases are well-structured and cover various configuration scenarios. Consider adding a few more test cases:
- Test with nil response
- Test with nil request
- Test with empty bodies
- Test with JSON bodies
This would improve coverage of edge cases.
118-123: Consider more precise assertionsUsing
strings.Containsfor assertions is flexible but might miss formatting issues or ordering problems. Consider using regex patterns or more structured assertion helpers for more precise verification of the output format.out := buf.String() for _, expect := range tc.expect { - if !strings.Contains(out, expect) { - t.Errorf("expected output to contain %q: %v", expect, out) - } + // Example of more precise assertion using regex + matched, err := regexp.MatchString(fmt.Sprintf(`(?m).*%s.*`, regexp.QuoteMeta(expect)), out) + if err != nil { + t.Fatalf("regex error: %v", err) + } + if !matched { + t.Errorf("expected output to match pattern containing %q: %v", expect, out) + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/printer/printer.go(2 hunks)pkg/printer/printer_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
pkg/printer/printer_test.go (3)
pkg/printer/printer.go (2)
NewPrinter(29-36)PrinterConfig(14-19)pkg/printer/formatter.go (2)
ColorFormatter(29-29)PlainFormatter(30-30)pkg/result/result.go (2)
Result(9-14)Metadata(16-18)
pkg/printer/printer.go (2)
pkg/printer/formatter.go (2)
Formatter(14-19)NewFormatter(21-27)pkg/result/result.go (4)
Result(9-14)RequestLine(26-30)StatusLine(20-24)Body(32-32)
🔇 Additional comments (2)
pkg/printer/printer.go (1)
39-42: LGTM!I see you've added a nil check for
res.Responseas suggested in the previous review comment. This is a good practice even if, as you mentioned, theClient.Doimplementation ensures it won't be nil.pkg/printer/printer_test.go (1)
14-47: Good test design for NewPrinterThe table-driven test pattern is a good choice, and the test cases cover both terminal and non-terminal scenarios. The temporary modification of the global
isTerminalvariable is properly restored withdefer.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/printer/printer.go (1)
71-78: Potential nil dereference for Request.
You dereferenceres.Response.Requestwithout a safety check. You’ve mentioned it’s guaranteed non-nil in your flow, so this is acceptable. However, consider a brief comment or doc note for future maintainers.pkg/printer/printer_test.go (1)
49-123: Comprehensive coverage but missing nil-response case.
The subtests thoroughly check different printing scenarios. Consider adding a case whereres.Responseisnilto confirm the fallback message “No HTTP response available.”pkg/printer/formatter.go (2)
80-113: Status line color logic.
Time-based and status-based color choices look good. For more robust classification, consider parsing status codes numerically—but this is fine for a typical range of HTTP statuses.
138-144: formatJson fallback message.
You return “unable to parse json” without the actual error. Consider providing the error details for easier debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/printer/formatter.go(1 hunks)pkg/printer/printer.go(2 hunks)pkg/printer/printer_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
pkg/printer/printer_test.go (3)
pkg/printer/printer.go (4)
IsTerminal(14-14)NewPrinter(29-42)PrinterConfig(16-21)Printer(23-27)pkg/printer/formatter.go (2)
ColorFormatter(21-21)PlainFormatter(22-22)pkg/result/result.go (2)
Result(9-14)Metadata(16-18)
pkg/printer/formatter.go (2)
pkg/result/result.go (2)
StatusLine(20-24)RequestLine(26-30)pkg/printer/colors.go (9)
Cyan(39-41)Color(3-3)ColorGreen(8-8)ColorCyan(12-12)ColorRed(7-7)ColorReset(6-6)ColorYellow(9-9)Colorize(15-17)ColorMagenta(11-11)
🔇 Additional comments (17)
pkg/printer/printer.go (7)
5-6: Imports look appropriate.
No issues found with the newly introduced imports. They are all used and serve valid purposes.Also applies to: 9-9, 11-11
14-15: Test-friendly IsTerminal approach.
DefiningIsTerminalas a variable enables easy stubbing in tests, which is a clean approach.
24-26: Struct fields are well-defined.
ThePrinterstruct effectively captures config, destination, and formatter.
29-42: Factory method looks straightforward.
UsingIsTerminal(int(os.Stdout.Fd()))to decide between colored or plain formatting is concise. No concurrency or error-handling concerns here.
44-69: Centralized printing logic is clear.
This method composes sections based on config and prints them together. Ifres.Responseisnil, it exits gracefully. All good.
80-86: Response headers printing is straightforward.
No issues noted. Consistently reuses formatter logic to show headers.
88-98: Body printing logic is solid.
Nicely handles both JSON and textual output. Straightforward, no performance concerns noted.pkg/printer/printer_test.go (2)
4-5: Test imports are valid.
All imports are relevant to test setup and assertions.Also applies to: 7-7, 9-9
14-47: TestNewPrinter verifies formatter selection.
The table-driven approach to mockIsTerminalis clean and does not raise any issues.pkg/printer/formatter.go (8)
14-20: Interface definition is concise.
The methods cover essential formatting tasks, aligning well with usage inprinter.go.
21-22: Basic structs for color vs. plain formatting.
Splitting these into distinct types is straightforward and keeps logic clean.
24-30: Headers method is consistent for both color & plain.
Delegation toformatHeadersis appropriate; no redundant code.
32-38: StatusLine method uses shared helper.
No issues. UsingformatStatusLineis consistent and avoids duplication.
40-46: RequestLine color vs. plain logic.
Same approach asStatusLine—clear separation, no code duplication.
48-54: JSON formatter logic.
Both implementations reuseformatJson, reducing duplication. No concerns.
56-78: Header formatting is well-handled.
Sorting keys for deterministic output is a nice touch. Also, color-coding is optional.
115-136: Request line color logic.
Method-based color approach is intuitive and easily extensible.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/printer/printer.go (2)
14-14: Consider makingIsTerminala function rather than a package-level variable.A function-based approach (e.g., a helper
func isTerminalOut() bool) can help avoid accidental mutations and makes it more explicit when terminal checks are performed.
44-69: Potential concurrency caveat.If multiple goroutines call
Printerinstance, the combined output may interleave. Consider documenting this or wrapping output in a synchronization mechanism if thread safety is required.pkg/printer/formatter.go (1)
138-145: Consider more typical JSON indentation.You currently indent with a single space per level (
" "). Using two or four spaces can improve readability in common tooling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/printer/formatter.go(1 hunks)pkg/printer/printer.go(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pkg/printer/formatter.go (2)
pkg/result/result.go (2)
StatusLine(20-24)RequestLine(26-30)pkg/printer/colors.go (9)
Cyan(39-41)Color(3-3)ColorGreen(8-8)ColorCyan(12-12)ColorRed(7-7)ColorReset(6-6)ColorYellow(9-9)Colorize(15-17)ColorMagenta(11-11)
🔇 Additional comments (16)
pkg/printer/printer.go (6)
5-11: Imports look good.These imports are appropriate for the new functionality involving terminal detection, output to
os.Stdout, and writing viaio.Writer.
24-26: Flexible struct fields.Introducing
config,destination, andformatterinPrinterprovides a clean separation of concerns for configuration, output destination, and formatting logic.
29-42: Good constructor pattern.
NewPrintercorrectly sets thedestinationtoos.Stdoutand chooses a formatter based onIsTerminal. This pattern simplifies instantiation and makes testing easier.
71-74: No explicit nil-check forResponse.Request.Based on your design, the
Requestshould always be available here. This is acceptable if you’re certain it cannot be nil in normal usage. Otherwise, you can add a defensive check for extra safety.
76-79: Response headers extraction.This function straightforwardly formats the response’s status line and headers. The logic is clean and consistent with the request headers printing.
80-90: Text vs. JSON body handling is concise.The
printBodymethod clearly distinguishes between JSON and text bodies. Providing a fallback to an empty string on no content is also reasonable.pkg/printer/formatter.go (10)
3-12: Imports are appropriate.These imports cover JSON encoding, HTTP handling, sorting, and string manipulation needed for formatting.
14-19: Well-defined Formatter interface.Having methods for each relevant part of HTTP output (status line, request line, headers, JSON) makes this interface easy to implement and extend.
21-22: Color vs. plain output strategy.Defining separate structs for color and plain formatting is clear. It keeps each approach distinct without adding branching logic.
24-30: Reusability inHeadersmethods.Both color and plain formatters delegate to
formatHeaders, avoiding code duplication and ensuring consistent output across implementations.
32-38: Status line logic separation.Similarly, delegating to
formatStatusLineensures shared logic for color and plain variants, improving maintainability.
40-46: Request line colorization is straightforward.The design cleanly separates the color-coded method logic from the rest of the format.
48-54: Unified JSON method.Both formatters call the same JSON formatting helper, reducing duplication while providing consistent output.
56-78: Efficient sorting approach informatHeaders.Collecting header keys and sorting them ensures predictable output ordering. The color-aware approach is well isolated in a single block.
80-113: Robust status coloring logic.Using
strings.HasPrefixfor status code checks is a simple technique that covers common ranges. The time-based color choice also adds a helpful performance indicator.
115-136: Method-based color coding.Switching on HTTP methods ensures consistent color usage. The defaulting to
ColorMagentafor unknown methods is a neat fallback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/printer/formatter.go (3)
21-22: Optional: unify implementations under a single struct.We already encapsulate colorization as a boolean flag in the helper functions. If you prefer reducing duplications, consider a single struct that holds a boolean for color output, simplifying maintenance.
48-50: Consider colorizing JSON inColorFormatter.Currently, both
ColorFormatterandPlainFormatterproduce identical JSON output. If you intend color-coded JSON, consider adding color highlighting to keys or braces for consistency with the rest of the color-coded outputs.Also applies to: 52-54
80-105: Make response time thresholds customizable or configurable.The current thresholds for coloring response times (500 ms for no color and 1 second for red) might be too rigid for some use cases. Exposing them as configuration or environment variables could make this feature more flexible.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/printer/formatter.go(1 hunks)pkg/printer/formatter_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/printer/formatter_test.go
🧰 Additional context used
🧬 Code Definitions (1)
pkg/printer/formatter.go (2)
pkg/result/result.go (2)
StatusLine(20-24)RequestLine(26-30)pkg/printer/colors.go (9)
Cyan(39-41)Color(3-3)ColorGreen(8-8)ColorCyan(12-12)ColorRed(7-7)ColorReset(6-6)ColorYellow(9-9)Colorize(15-17)ColorMagenta(11-11)
🔇 Additional comments (2)
pkg/printer/formatter.go (2)
14-19: Well-structured interface for extensibility.Declaring the
Formatterinterface centralizes formatting concerns and makes it easy to add more formatters in the future.
138-144: Edge case handling for non-JSON or cyclic structures.While an error case is handled, callers might benefit from a broader approach to reporting or retrying failed JSON conversions (e.g., partial fallback or structured logs). Confirm this minimal error message meets your needs.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Formatterstruct for improved formatting of various output types.Chore