-
Notifications
You must be signed in to change notification settings - Fork 42
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
JSONL support #487
JSONL support #487
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 |
---|---|---|
|
@@ -21,14 +21,18 @@ type Colorer func(string, ...interface{}) string | |
|
||
type Printer struct { | ||
// Must always be present | ||
Output io.Writer | ||
JSON bool | ||
Output io.Writer | ||
JSON bool | ||
// This is unset/empty in JSONL mode | ||
JSONIndent string | ||
JSONPayloadShorthand bool | ||
// Only used for non-JSON, defaults to RFC3339 | ||
FormatTime func(time.Time) string | ||
// Only used for non-JSON, defaults to color.Magenta | ||
TableHeaderColorer Colorer | ||
|
||
listMode bool | ||
listModeFirstJSON bool // True until first JSON printed | ||
} | ||
|
||
// Ignored during JSON output | ||
|
@@ -50,6 +54,42 @@ func (p *Printer) Printlnf(s string, v ...any) { | |
p.Println(fmt.Sprintf(s, v...)) | ||
} | ||
|
||
// When called for JSON with indent, this will create an initial bracket and | ||
// make sure all [Printer.PrintStructured] calls get commas properly to appear | ||
// as a list (but the indention and multiline posture of the JSON remains). When | ||
// called for JSON without indent, this will make sure all | ||
// [Printer.PrintStructured] is on its own line (i.e. JSONL mode). When called | ||
// for non-JSON, this is a no-op. | ||
Comment on lines
+57
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This (and the if checks below) says to me that rather than using an empty There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
More like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer that I think but it's not a big deal. I'll approve and leave it up to you. Lack up sum types 😢 |
||
// | ||
// [Printer.EndList] must be called at the end. If this is called twice it will | ||
// panic. This and the end call are not safe for concurrent use. | ||
func (p *Printer) StartList() { | ||
if p.listMode { | ||
panic("already in list mode") | ||
} | ||
p.listMode, p.listModeFirstJSON = true, true | ||
// Write initial bracket when non-jsonl | ||
if p.JSON && p.JSONIndent != "" { | ||
// Don't need newline, we count on initial object to do that | ||
p.Output.Write([]byte("[")) | ||
} | ||
} | ||
|
||
// Must be called after [Printer.StartList] or will panic. See Godoc on that | ||
// function for more details. | ||
func (p *Printer) EndList() { | ||
if !p.listMode { | ||
panic("not in list mode") | ||
} | ||
p.listMode, p.listModeFirstJSON = false, false | ||
// Write ending bracket when non-jsonl | ||
if p.JSON && p.JSONIndent != "" { | ||
// We prepend a newline because non-jsonl list mode doesn't do so after each | ||
// line to help with commas | ||
p.Output.Write([]byte("\n]\n")) | ||
} | ||
} | ||
|
||
type StructuredOptions struct { | ||
// Derived if not present. Ignored for JSON printing. | ||
Fields []string | ||
|
@@ -171,19 +211,40 @@ func (p *Printer) writef(s string, v ...any) { | |
} | ||
|
||
func (p *Printer) printJSON(v any, options StructuredOptions) error { | ||
// Before printing, if we're in non-jsonl list mode, we must append a comma | ||
// and a newline if we're not the first JSON seen. | ||
nonJSONLListMode := p.listMode && p.JSON && p.JSONIndent != "" | ||
if nonJSONLListMode { | ||
var prepend string | ||
if p.listModeFirstJSON { | ||
p.listModeFirstJSON = false | ||
prepend = "\n" | ||
} else { | ||
prepend = ",\n" | ||
} | ||
if _, err := p.Output.Write([]byte(prepend)); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
// Print JSON | ||
shorthandPayloads := p.JSONPayloadShorthand | ||
if options.OverrideJSONPayloadShorthand != nil { | ||
shorthandPayloads = *options.OverrideJSONPayloadShorthand | ||
} | ||
b, err := p.jsonVal(v, p.JSONIndent, shorthandPayloads) | ||
if err != nil { | ||
if b, err := p.jsonVal(v, p.JSONIndent, shorthandPayloads); err != nil { | ||
return err | ||
} else if _, err := p.Output.Write(b); err != nil { | ||
return err | ||
} | ||
_, err = p.Output.Write(b) | ||
if err == nil { | ||
_, err = p.Output.Write([]byte("\n")) | ||
|
||
// Do not print a newline if in non-jsonl list mode | ||
if !nonJSONLListMode { | ||
if _, err := p.Output.Write([]byte("\n")); err != nil { | ||
return err | ||
} | ||
} | ||
return err | ||
return nil | ||
} | ||
|
||
func (p *Printer) jsonVal(v any, indent string, shorthandPayloads bool) ([]byte, error) { | ||
|
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.
I'm thinking maybe we should use the term "array" rather than "list" in the code since that's the correct JSON terminology.
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.
list
is what we call the commands and I don't want users to even have to worry about whether JSON is enabled or not. I want people to say whether their jsonl list is starting/ending too. From a caller POV, these calls are general purpose, not only JSON (despite my comments above them, maybe I should remove those). Often people don't think of arrays as lazy/streaming.Having said this, if the word "array" is important enough, I can change to get PR approval.
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.
Thanks, that's fine, especially if there's a connection with the use of "list" as a verb in the CLI.