feat(shell-api, cli-repl): pass print type as part of the onPrint callback; do not limit the output of printjson MONGOSH-955 #1356
Conversation
…lback; do not limit the output of printjson
gribnoysup
force-pushed
the
mongosh-955-printjson-is-shelljson
branch
from
1ca6914 to
c490a94
Compare
| maxStringLength: Infinity | ||
| } | ||
| : undefined; | ||
| const joined = values |
There was a problem hiding this comment.
Old printjson also ignores everything except first argument, I am not sure whether we want to replicate this behavior or not
| // infinity | ||
| type === 'printjson' | ||
| ? { | ||
| colors: false, |
There was a problem hiding this comment.
I don’t think the syntax highlighting was really brought up as a problem, right? I’m not sure if there’s really much point in disabling it, we only use colors when printing to outputs that support it anyway.
There was a problem hiding this comment.
Yeah, I can remove this, I was thinking that if people want us to add support for this because they are using this programmatically, maybe it's safer to remove any unexpected symbols from the output, which ansi color codes might be, but I might be just over cautious here
There was a problem hiding this comment.
Yeah, I think our color support detection code is pretty solid and we haven’t ever received a complaint about it, so I think it’s safe to leave the color support in the default mode here.
Also, to be clear, this PR currently wouldn’t give users programmatically usable output. We did discuss the option of making printjson() print actual (E)JSON, but ultimately felt that that would also not really be what users want, despite the name and everything.
There was a problem hiding this comment.
Oh, yeah, totally! I guess I meant usable in users sense 😄 I agree that otherwise this method is pretty confusing and should probably be avoided
| @@ -74,7 +74,7 @@ export interface EvaluationListener extends Partial<ConfigProvider<ShellUserConf | |||
| /** | |||
| * Called when print() or printjson() is run from the shell. | |||
| */ | |||
| onPrint?: (value: ShellResult[]) => Promise<void> | void; | |||
| onPrint?: (value: ShellResult[], type: 'print' | 'printjson') => Promise<void> | void; | |||
There was a problem hiding this comment.
Do we need/want to update browser-repl as well? I’m not sure what that would even look like.
There was a problem hiding this comment.
I am not sure that we would want to, yeah, doesn't look like this would make sense in browser-repl context to me too
Here's how the output looks for
printjson(Array.from({length: 1000}, (_, idx) => ({_id: ObjectId(), idx})));now:TODO: