-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
A different way of fixing escaping #14186
Changes from 7 commits
9bc647a
8ca1b52
3b56ba2
d73c759
60fc1a3
d959378
d3f33b9
75374a5
2534df5
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 |
---|---|---|
@@ -0,0 +1 @@ | ||
Do not escape output in the actual ipynb file. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,11 +40,7 @@ import { KernelConnectionMetadata } from './kernels/types'; | |
|
||
// tslint:disable-next-line: no-require-imports | ||
import cloneDeep = require('lodash/cloneDeep'); | ||
// tslint:disable-next-line: no-require-imports | ||
import escape = require('lodash/escape'); | ||
// tslint:disable-next-line: no-require-imports | ||
import unescape = require('lodash/unescape'); | ||
import { concatMultilineString, formatStreamText } from '../../../datascience-ui/common'; | ||
import { concatMultilineString, formatStreamText, splitMultilineString } from '../../../datascience-ui/common'; | ||
import { RefBool } from '../../common/refBool'; | ||
import { PythonEnvironment } from '../../pythonEnvironments/info'; | ||
import { getInterpreterFromKernelConnectionMetadata, isPythonKernelConnection } from './kernels/helpers'; | ||
|
@@ -787,12 +783,12 @@ export class JupyterNotebookBase implements INotebook { | |
outputs.forEach((o) => { | ||
if (o.output_type === 'stream') { | ||
const stream = o as nbformat.IStream; | ||
result = result.concat(formatStreamText(unescape(concatMultilineString(stream.text, true)))); | ||
result = result.concat(formatStreamText(concatMultilineString(stream.text, true))); | ||
} else { | ||
const data = o.data; | ||
if (data && data.hasOwnProperty('text/plain')) { | ||
// tslint:disable-next-line:no-any | ||
result = result.concat(unescape((data as any)['text/plain'])); | ||
result = result.concat((data as any)['text/plain']); | ||
} | ||
} | ||
}); | ||
|
@@ -1206,12 +1202,7 @@ export class JupyterNotebookBase implements INotebook { | |
|
||
private addToCellData = ( | ||
cell: ICell, | ||
output: | ||
| nbformat.IUnrecognizedOutput | ||
| nbformat.IExecuteResult | ||
| nbformat.IDisplayData | ||
| nbformat.IStream | ||
| nbformat.IError, | ||
output: nbformat.IExecuteResult | nbformat.IDisplayData | nbformat.IStream | nbformat.IError, | ||
clearState: RefBool | ||
) => { | ||
const data: nbformat.ICodeCell = cell.data as nbformat.ICodeCell; | ||
|
@@ -1237,7 +1228,10 @@ export class JupyterNotebookBase implements INotebook { | |
) { | ||
// Check our length on text output | ||
if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) { | ||
msg.content.data['text/plain'] = escape(trimFunc(msg.content.data['text/plain'] as string)); | ||
msg.content.data['text/plain'] = splitMultilineString( | ||
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. The other spot where output was different is saving as an array instead of a single string. Jupyter makes stuff into arrays. 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 seems ok, but I thought that Jupyter did either a string array or a single string. Does it have to be the string array? 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. Should be fine I guess. If it's single string the split won't do anything. |
||
// tslint:disable-next-line: no-any | ||
trimFunc(concatMultilineString(msg.content.data['text/plain'] as any)) | ||
); | ||
} | ||
|
||
this.addToCellData( | ||
|
@@ -1265,14 +1259,15 @@ export class JupyterNotebookBase implements INotebook { | |
reply.payload.forEach((o) => { | ||
if (o.data && o.data.hasOwnProperty('text/plain')) { | ||
// tslint:disable-next-line: no-any | ||
const str = (o.data as any)['text/plain'].toString(); | ||
const data = escape(trimFunc(str)) as string; | ||
const str = concatMultilineString((o.data as any)['text/plain']); // NOSONAR | ||
const data = trimFunc(str); | ||
this.addToCellData( | ||
cell, | ||
{ | ||
// Mark as stream output so the text is formatted because it likely has ansi codes in it. | ||
output_type: 'stream', | ||
text: data, | ||
text: splitMultilineString(data), | ||
name: 'stdout', | ||
metadata: {}, | ||
execution_count: reply.execution_count | ||
}, | ||
|
@@ -1313,23 +1308,25 @@ export class JupyterNotebookBase implements INotebook { | |
? data.outputs[data.outputs.length - 1] | ||
: undefined; | ||
if (existing) { | ||
// tslint:disable-next-line:restrict-plus-operands | ||
existing.text = existing.text + escape(msg.content.text); | ||
const originalText = formatStreamText(concatMultilineString(existing.text)); | ||
const originalText = formatStreamText( | ||
// tslint:disable-next-line: no-any | ||
`${concatMultilineString(existing.text as any)}${concatMultilineString(msg.content.text)}` | ||
); | ||
originalTextLength = originalText.length; | ||
existing.text = trimFunc(originalText); | ||
trimmedTextLength = existing.text.length; | ||
const newText = trimFunc(originalText); | ||
trimmedTextLength = newText.length; | ||
existing.text = splitMultilineString(newText); | ||
} else { | ||
const originalText = formatStreamText(concatMultilineString(escape(msg.content.text))); | ||
const originalText = formatStreamText(concatMultilineString(msg.content.text)); | ||
originalTextLength = originalText.length; | ||
// Create a new stream entry | ||
const output: nbformat.IStream = { | ||
output_type: 'stream', | ||
name: msg.content.name, | ||
text: trimFunc(originalText) | ||
text: [trimFunc(originalText)] | ||
}; | ||
data.outputs = [...data.outputs, output]; | ||
trimmedTextLength = output.text.length; | ||
trimmedTextLength = output.text[0].length; | ||
cell.data = data; | ||
} | ||
|
||
|
@@ -1338,23 +1335,16 @@ export class JupyterNotebookBase implements INotebook { | |
// the output is trimmed and what setting changes that. | ||
// * If data.metadata.tags is undefined, define it so the following | ||
// code is can rely on it being defined. | ||
if (data.metadata.tags === undefined) { | ||
data.metadata.tags = []; | ||
} | ||
|
||
data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend'); | ||
|
||
if (trimmedTextLength < originalTextLength) { | ||
if (data.metadata.tags === undefined) { | ||
data.metadata.tags = []; | ||
} | ||
data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend'); | ||
data.metadata.tags.push('outputPrepend'); | ||
} | ||
} | ||
|
||
private handleDisplayData(msg: KernelMessage.IDisplayDataMsg, clearState: RefBool, cell: ICell) { | ||
// Escape text output | ||
if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) { | ||
msg.content.data['text/plain'] = escape(msg.content.data['text/plain'] as string); | ||
} | ||
|
||
const output: nbformat.IDisplayData = { | ||
output_type: 'display_data', | ||
data: msg.content.data, | ||
|
@@ -1402,10 +1392,14 @@ export class JupyterNotebookBase implements INotebook { | |
private handleError(msg: KernelMessage.IErrorMsg, clearState: RefBool, cell: ICell) { | ||
const output: nbformat.IError = { | ||
output_type: 'error', | ||
ename: escape(msg.content.ename), | ||
evalue: escape(msg.content.evalue), | ||
traceback: msg.content.traceback.map(escape) | ||
ename: msg.content.ename, | ||
evalue: msg.content.evalue, | ||
traceback: msg.content.traceback | ||
}; | ||
if (msg.content.hasOwnProperty('transient')) { | ||
// tslint:disable-next-line: no-any | ||
output.transient = (msg.content as any).transient; | ||
} | ||
this.addToCellData(cell, output, clearState); | ||
cell.state = CellState.error; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,8 @@ import { getRichestMimetype, getTransform, isIPyWidgetOutput, isMimeTypeSupporte | |
|
||
// tslint:disable-next-line: no-var-requires no-require-imports | ||
const ansiToHtml = require('ansi-to-html'); | ||
// tslint:disable-next-line: no-var-requires no-require-imports | ||
const lodashEscape = require('lodash/escape'); | ||
|
||
// tslint:disable-next-line: no-require-imports no-var-requires | ||
const cloneDeep = require('lodash/cloneDeep'); | ||
|
@@ -328,7 +330,7 @@ export class CellOutput extends React.Component<ICellOutputProps> { | |
// tslint:disable-next-line: no-any | ||
const text = (input as any)['text/plain']; | ||
input = { | ||
'text/html': text // XML tags should have already been escaped. | ||
'text/html': lodashEscape(text) | ||
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. Now the escaping happens when we format the output. Should have done it this way from the beginning. |
||
}; | ||
} else if (output.output_type === 'stream') { | ||
mimeType = 'text/html'; | ||
|
@@ -337,7 +339,7 @@ export class CellOutput extends React.Component<ICellOutputProps> { | |
renderWithScrollbars = true; | ||
// Sonar is wrong, TS won't compile without this AS | ||
const stream = output as nbformat.IStream; // NOSONAR | ||
const concatted = concatMultilineString(stream.text); | ||
const concatted = lodashEscape(concatMultilineString(stream.text)); | ||
input = { | ||
'text/html': concatted // XML tags should have already been escaped. | ||
}; | ||
|
@@ -363,14 +365,18 @@ export class CellOutput extends React.Component<ICellOutputProps> { | |
const error = output as nbformat.IError; // NOSONAR | ||
try { | ||
const converter = new CellOutput.ansiToHtmlClass(CellOutput.getAnsiToHtmlOptions()); | ||
const trace = error.traceback.length ? converter.toHtml(error.traceback.join('\n')) : error.evalue; | ||
// Modified traceback may exist. If so use that instead. It's only at run time though | ||
const traceback: string[] = error.transient | ||
? (error.transient as string[]) | ||
: error.traceback.map(lodashEscape); | ||
const trace = traceback ? converter.toHtml(traceback.join('\n')) : error.evalue; | ||
input = { | ||
'text/html': trace | ||
}; | ||
} catch { | ||
// This can fail during unit tests, just use the raw data | ||
input = { | ||
'text/html': error.evalue | ||
'text/html': lodashEscape(error.evalue) | ||
}; | ||
} | ||
} else if (input) { | ||
|
@@ -395,6 +401,12 @@ export class CellOutput extends React.Component<ICellOutputProps> { | |
data = fixMarkdown(concatMultilineString(data as nbformat.MultilineString, true), true); | ||
} | ||
|
||
// Make sure text output is escaped (nteract texttransform won't) | ||
if (mimeType === 'text/plain') { | ||
data = lodashEscape(data.toString()); | ||
mimeType = 'text/html'; | ||
} | ||
|
||
return { | ||
isText, | ||
isError, | ||
|
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.
This is also changing the output that would normally be there so I made it transient. It will only show up in while the interactive window is open. Exporting to a notebook won't show it anymore (which I think is probably what we want)
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.
Yeah, that sounds better as it's specific to us.