JSONL support #487
JSONL support #487
Conversation
| @@ -70,6 +70,10 @@ func (c TemporalBatchListCommand) run(cctx *CommandContext, args []string) error | |||
| } | |||
| defer cl.Close() | |||
|
|
|||
| // This is a listing command subject to json vs jsonl rules | |||
| cctx.Printer.StartList() | |||
| defer cctx.Printer.EndList() | |||
There was a problem hiding this comment.
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.
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.
Thanks, that's fine, especially if there's a connection with the use of "list" as a verb in the CLI.
| // 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. |
There was a problem hiding this comment.
This (and the if checks below) says to me that rather than using an empty JSONIndent as a special indication to do JSONL, we should instead just have the JSON field be JSON | JSONL and just be explicit about what mode we're in.
There was a problem hiding this comment.
we should instead just have the
JSONfield beJSON | JSONL
More like JSON | JSONL | Neither, but it's ugly in Go. I considered this (and I considered two booleans), but JSONIndent is how Go does it (where empty string means no multiline, ref https://pkg.go.dev/encoding/json#Encoder.SetIndent). Throughout the Go JSON printing code they check whether indentation is set. And this option gives the caller the freedom to set the indent they want, even though we're the caller. However, if the way Go did this is too confusing, I can make this a Go enum and add a isJSON() that checks for those two values.
There was a problem hiding this comment.
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 😢
|
Merging, but I do not hold the use of empty-jsonindent-means-jsonl nor the use of list vs array as a strong opinion if anyone really wants them changed. |
What was changed
--output jsonloption which removes indentation/newlines from JSON outputStartListandEndList--output jsonmode, this will add[at the beginning, commas before newline of each successive object, and]at the end so that it will parse as an array--output jsonlmode, this will do nothing special (indention is already off, just allows normal use of repeated JSON values)StartListandEndListtoworkflow list,batch list, andcluster list(note, there may be an outstanding flaky batch test we are researching)
Checklist
workflow list#448