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 all commits
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
29 changes: 25 additions & 4 deletions packages/cli-repl/src/mongosh-repl.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,31 @@ describe('MongoshNodeRepl', () => {
expect(error.message).to.include('surprise!!!');
});

it('prints values when asked to', async() => {
input.write('print("see this?"); 42\n');
await waitEval(bus);
expect(output).to.include('see this?');
context('print / printjson', () => {
it('prints values when asked to', async() => {
output = '';
input.write('print("see this?"); 42\n');
await waitEval(bus);
expect(output).to.include('see this?');
});

it('respects user format output settings when print is used', async() => {
input.write('config.set("inspectDepth", 0);\n');
await waitEval(bus);
output = '';
input.write('print({ a: { b: { c: 1 } } });\n');
await waitEval(bus);
expect(output).to.include('{ a: [Object] }');
});

it('prints out content in a legacy printjson format', async() => {
input.write('config.set("inspectDepth", 0);\n');
await waitEval(bus);
output = '';
input.write('printjson({ a: { b: { c: 1 } } });\n');
await waitEval(bus);
expect(output).to.include('{\n a: {\n b: {\n c: 1\n }\n }\n}');
});
});

it('forwards telemetry config requests', async() => {
Expand Down
41 changes: 34 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,37 @@ 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: start every object on the new
// line, and set all the collapse options threshold to infinity
type === 'printjson'
? {
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 +725,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 +759,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
16 changes: 11 additions & 5 deletions packages/shell-api/src/shell-api.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -671,12 +671,18 @@ describe('ShellApi', () => {
// eslint-disable-next-line no-loop-func
describe(cmd, () => {
it('prints values', async() => {
evaluationListener.onPrint.resolves();
// eslint-disable-next-line chai-friendly/no-unused-expressions
evaluationListener.onPrint?.resolves();
await instanceState.context[cmd](1, 2);
expect(evaluationListener.onPrint).to.have.been.calledWith([
{ printable: 1, rawValue: 1, type: null },
{ printable: 2, rawValue: 2, type: null }
]);
expect(
evaluationListener.onPrint
).to.have.been.calledOnceWithExactly(
[
{ printable: 1, rawValue: 1, type: null },
{ printable: 2, rawValue: 2, type: null }
],
cmd
);
});
});
}
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