Skip to content
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

feat(shell-api, cli-repl): pass print type as part of the onPrint callback; do not limit the output of printjson MONGOSH-955 #1356

Merged
merged 2 commits into from
Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/cli-repl/src/format-output.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ type EvaluationResult = {
type?: string | null;
};

type FormatOptions = {
export type FormatOptions = {
colors: boolean;
compact?: boolean | number;
depth?: number;
Expand Down
43 changes: 36 additions & 7 deletions packages/cli-repl/src/mongosh-repl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { MONGOSH_WIKI, TELEMETRY_GREETING_MESSAGE } from './constants';
import formatOutput, { formatError } from './format-output';
import { makeMultilineJSIntoSingleLine } from '@mongosh/js-multiline-to-singleline';
import { LineByLineInput } from './line-by-line-input';
import type { FormatOptions } from './format-output';

/**
* All CLI flags that are useful for {@link MongoshNodeRepl}.
Expand Down Expand Up @@ -646,17 +647,39 @@ class MongoshNodeRepl implements EvaluationListener {
* @param result A ShellResult (or similar) object.
* @returns The pretty-printed version of the input.
*/
formatShellResult(result: { type: null | string, printable: any }): string {
return this.formatOutput({ type: result.type, value: result.printable });
formatShellResult(
result: { type: null | string; printable: any },
extraFormatOptions: Partial<FormatOptions> = {}
): string {
return this.formatOutput(
{ type: result.type, value: result.printable },
extraFormatOptions
);
}

/**
* Called when print(), console.log() etc. are called from the shell.
*
* @param values A list of values to be printed.
*/
onPrint(values: ShellResult[]): void {
const joined = values.map((value) => this.formatShellResult(value)).join(' ');
onPrint(values: ShellResult[], type: 'print' | 'printjson'): void {
const extraOptions: Partial<FormatOptions> | undefined =
// MONGOSH-955: when `printjson()` is called in mongosh, we will try to
// replicate the format of the old shell: disable colors, start every
// object on the new line, and set all the collapse options threshold to
// infinity
type === 'printjson'
? {
colors: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

compact: false,
depth: Infinity,
maxArrayLength: Infinity,
maxStringLength: Infinity
}
: undefined;
const joined = values
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old printjson also ignores everything except first argument, I am not sure whether we want to replicate this behavior or not

.map((value) => this.formatShellResult(value, extraOptions))
.join(' ');
this.output.write(joined + '\n');
}

Expand Down Expand Up @@ -704,8 +727,14 @@ class MongoshNodeRepl implements EvaluationListener {
* @param value A value, together with optional type information.
* @returns The pretty-printed version of the input.
*/
formatOutput(value: { value: any, type?: string | null }): string {
return formatOutput(value, this.getFormatOptions());
formatOutput(
value: { value: any; type?: string | null },
extraFormatOptions: Partial<FormatOptions> = {}
): string {
return formatOutput(value, {
...this.getFormatOptions(),
...extraFormatOptions
});
}

/**
Expand All @@ -732,7 +761,7 @@ class MongoshNodeRepl implements EvaluationListener {
/**
* Provides the current set of output formatting options used for this shell.
*/
getFormatOptions(): { colors: boolean, compact: number | boolean, depth: number, showStackTraces: boolean, bugReportErrorMessageInfo?: string } {
getFormatOptions(): FormatOptions {
const output = this.output as WriteStream;
return {
colors: this._runtimeState?.repl?.useColors ??
Expand Down
15 changes: 10 additions & 5 deletions packages/shell-api/src/shell-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,17 +298,22 @@ export default class ShellApi extends ShellApiClass {
return await promisify(setTimeout)(ms);
}

private async _print(origArgs: any[], type: 'print' | 'printjson'): Promise<void> {
const { evaluationListener } = this._instanceState;
const args: ShellResult[] = await Promise.all(
origArgs.map((arg) => toShellResult(arg))
);
await evaluationListener.onPrint?.(args, type);
}

@returnsPromise
async print(...origArgs: any[]): Promise<void> {
const { evaluationListener } = this._instanceState;
const args: ShellResult[] =
await Promise.all(origArgs.map(arg => toShellResult(arg)));
await evaluationListener.onPrint?.(args);
await this._print(origArgs, 'print');
}

@returnsPromise
async printjson(...origArgs: any[]): Promise<void> {
return this.print(...origArgs);
await this._print(origArgs, 'printjson');
}

@directShellCommand
Expand Down
2 changes: 1 addition & 1 deletion packages/shell-api/src/shell-instance-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need/want to update browser-repl as well? I’m not sure what that would even look like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Called when e.g. passwordPrompt() is called from the shell.
Expand Down