feat: add tabular output to HumanRenderer
#19
Conversation
Introduces `go-pretty/v6/table` to keep human readable output aligned without tedious use of `fmt.Sprintf()`, previously found in `formatRecord()`. Additionally updates the `Renderer` interface as table rows need to be submitted before writing them to stdout.
There was a problem hiding this comment.
Left a couple of comments—great work so far on replacing the flaky tab writing we currently have! I think there are a few areas we can improve, such as forcing the empty interface as a return value and avoiding breaking the interface. Thanks for your work on this @mmourick!
| @@ -149,9 +149,10 @@ var ( | |||
|
|
|||
| for _, m := range messages { | |||
| for _, record := range m.Answer { | |||
| v.Render(args[0], record) | |||
| v.AddRecord(args[0], record) | |||
There was a problem hiding this comment.
From a consumer’s perspective, it’s a bit unclear what exactly is being added or appended. I’m still considering a cleaner alternative and will get back on this. What do you think?
There was a problem hiding this comment.
It made sense to me, as a record is added to view v by means of AddRecord().
| // formatRecord generates a human-readable string representing a DNS record with colors. | ||
| func formatRecord(domainName string, answer dns.RR) string { | ||
| // formatRecord generates human-readable strings representing a DNS record with colors. | ||
| func formatRecord(domain string, answer dns.RR) []interface{} { |
There was a problem hiding this comment.
Can’t we preserve returning a string, with the conversion to []interface{} handled further down the call stack where that empty type is required? This keeps this function intact and more easily testable. There’s no reason for this function to return a []interface{} other than fulfilling a specific consumer requirement elsewhere. Or am I overseeing something?
There was a problem hiding this comment.
I needed to change the return type (and the implementation, somewhat) of this function, because the table handles the formatting for rows containing a variadic amount of elements, and these elements need to be provided separately, namely via []interface{} .
I had an earlier implementation of this function that returned []string, however that caused an additional loop since I wouldn't be able to use the ... notation to unpack the values and add them to a row.
|
|
||
| Thank you for your contribution! | ||
| `, recordType, recordType) | ||
| return []interface{}{fmt.Sprintf(` |
There was a problem hiding this comment.
I believe preserving a multiline string—would improve readability from the user’s perspective. There's definitely room for improvement in the existing error message we return now.
There was a problem hiding this comment.
Yeah the current setup is not ideal. I shortened it because the multiline message severely messes up the formatting of the output, and while this current setup looks a bit better, perhaps this should be treated as an error?
| @@ -182,7 +182,7 @@ func TestFormatRecord(t *testing.T) { | |||
| os.Setenv("NO_COLOR", "true") // Disable colors for easier testing | |||
|
|
|||
| r := formatRecord(domain, record) | |||
| assert.Equal(t, "A\texample.com.\t03m42s\t127.0.0.1", r) | |||
| assert.Equal(t, []interface{}{"A", "example.com", "03m42s", "127.0.0.1"}, r) | |||
There was a problem hiding this comment.
This ties back to an earlier comment on the use of []interface{}. It highlights the issues that arise from forcing an empty type, as any other consumer of that function that doesn’t need it is now unnecessarily burdened with handling it.
| @@ -69,3 +90,8 @@ func (v *JSONRenderer) Render(domain string, record dns.RR) { | |||
|
|
|||
| v.view.Output("Successful query", params...) | |||
| } | |||
|
|
|||
| // JSONRender is not buffered, so no need to flush | |||
| func (v *JSONRenderer) Render() { | |||
There was a problem hiding this comment.
The new Render method highlights a lack of true shared behavior between the renderers, conflicting with Go’s design philosophy for interfaces: "Don’t design with interfaces, discover them." This simply tries to say: introduce an interface when you discover genuine shared behavior—such as two renderers that do something similar, like rendering DNS records.
Previously, this was the case—each Renderer (JSON or Human) knew how to render a DNS record back to the TTY via a stream.
There was a problem hiding this comment.
Sadly, the human friendly renderer diverges a bit from the previous interface.
If preferred, the JSON rendered could adopt some likeness from the human renderer, storing the records in a buffer until Render() is called?
The table implementation otherwise cannot be implemented if it has to abide by the unmodified interface.
Perhaps another solution would be to pass the entire the entire answer []dns.RR to the Render() method. That would still require updating the interface, but eliminates the need for a second function in the interface.
|
|
||
| // Render writes the human-readable table to the output stream | ||
| func (v *HumanRenderer) Render() { | ||
| v.t.Render() |
There was a problem hiding this comment.
This reads a bit odd—I’m guessing it’s because t (table) also has a Render method. However, it made me double-check t to ensure we weren’t unintentionally calling something recursively.
There was a problem hiding this comment.
Correct, t has it's own Render() method. perhaps t should be renamed to table.
|
Hey @mmourick, After our discussion above, I revisited the Personally, I’d prefer to retain as much of the original rendering (view/stream code) as possible. If you’re considering a more substantial change, it might be worth to align on the best approach before moving forward. Thanks! |
Introduces
go-pretty/v6/tableto keep human readable output aligned without tedious use offmt.Sprintf(), previously found informatRecord().Additionally updates the
Rendererinterface as table rows need to be submitted before writing them to stdout.Fixes #11