From 1e90e88d4bde2f79b1fddb88ece11439a3df155a Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Fri, 7 Dec 2018 12:14:43 -0800 Subject: [PATCH] Fix #3481 Notebook: Markdown coloring appears incorrect (#3499) - Set markdown as language for markdown cell - Fix issue where after loading language from cell metadata, always override it. This made the "language" feature irrelevant in the cell. - Fixed tests with new behavior (assumption: cell-level language overrides notebook-level definition) and added new test to cover this too --- src/sql/parts/notebook/models/cell.ts | 62 ++++++++++++++++--- src/sqltest/parts/notebook/model/cell.test.ts | 22 ++++++- 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/src/sql/parts/notebook/models/cell.ts b/src/sql/parts/notebook/models/cell.ts index 0be6bbd7c9..1795e3076a 100644 --- a/src/sql/parts/notebook/models/cell.ts +++ b/src/sql/parts/notebook/models/cell.ts @@ -38,15 +38,15 @@ export class CellModel implements ICellModel { constructor(private factory: IModelFactory, cellData?: nb.ICellContents, private _options?: ICellModelOptions) { this.id = `${modelId++}`; CellModel.CreateLanguageMappings(); - // Do nothing for now if (cellData) { + // Read in contents if available this.fromJSON(cellData); } else { this._cellType = CellTypes.Code; this._source = ''; } this._isEditMode = this._cellType !== CellTypes.Markdown; - this.setDefaultLanguage(); + this.ensureDefaultLanguage(); if (_options && _options.isTrusted) { this._isTrusted = true; } else { @@ -284,7 +284,7 @@ export class CellModel implements ICellModel { } this._cellType = cell.cell_type; this._source = Array.isArray(cell.source) ? cell.source.join('') : cell.source; - this._language = (cell.metadata && cell.metadata.language) ? cell.metadata.language : 'python'; + this.setLanguageFromContents(cell); if (cell.outputs) { for (let output of cell.outputs) { // For now, we're assuming it's OK to save these as-is with no modification @@ -293,6 +293,15 @@ export class CellModel implements ICellModel { } } + private setLanguageFromContents(cell: nb.ICellContents): void { + if (cell.cell_type === CellTypes.Markdown) { + this._language = 'markdown'; + } else if (cell.metadata && cell.metadata.language) { + this._language = cell.metadata.language; + } + // else skip, we set default language anyhow + } + private addOutput(output: nb.ICellOutput) { this._normalize(output); this._outputs.push(output); @@ -327,8 +336,32 @@ export class CellModel implements ICellModel { return undefined; } - private setDefaultLanguage(): void { - this._language = 'python'; + /** + * Ensures there is a default language set, if none was already defined. + * Will read information from the overall Notebook (passed as options to the model), or + * if all else fails default back to python. + * + */ + private ensureDefaultLanguage(): void { + // See if language is already set / is known based on cell type + if (this.hasLanguage()) { + return; + } + if (this._cellType === CellTypes.Markdown) { + this._language = 'markdown'; + return; + } + + // try set it based on overall Notebook language + this.trySetLanguageFromLangInfo(); + + // fallback to python + if (!this._language) { + this._language = 'python'; + } + } + + private trySetLanguageFromLangInfo() { // In languageInfo, set the language to the "name" property // If the "name" property isn't defined, check the "mimeType" property // Otherwise, default to python as the language @@ -338,16 +371,25 @@ export class CellModel implements ICellModel { // check the LanguageMapping to determine if a mapping is necessary (example 'pyspark' -> 'python') if (CellModel.LanguageMapping[languageInfo.name]) { this._language = CellModel.LanguageMapping[languageInfo.name]; - } else { + } + else { this._language = languageInfo.name; } - } else if (languageInfo.mimetype) { + } + else if (languageInfo.mimetype) { this._language = languageInfo.mimetype; } } - let mimeTypePrefix = 'x-'; - if (this._language.includes(mimeTypePrefix)) { - this._language = this._language.replace(mimeTypePrefix, ''); + + if (this._language) { + let mimeTypePrefix = 'x-'; + if (this._language.includes(mimeTypePrefix)) { + this._language = this._language.replace(mimeTypePrefix, ''); + } } } + + private hasLanguage(): boolean { + return !!this._language; + } } diff --git a/src/sqltest/parts/notebook/model/cell.test.ts b/src/sqltest/parts/notebook/model/cell.test.ts index b90298fbc5..2d602faa35 100644 --- a/src/sqltest/parts/notebook/model/cell.test.ts +++ b/src/sqltest/parts/notebook/model/cell.test.ts @@ -93,7 +93,7 @@ describe('Cell Model', function (): void { let cellData: nb.ICellContents = { cell_type: CellTypes.Code, source: 'print(\'1\')', - metadata: { language: 'python'}, + metadata: { }, execution_count: 1 }; @@ -105,6 +105,22 @@ describe('Cell Model', function (): void { let cell = factory.createCell(cellData, { notebook: notebookModel, isTrusted: false }); should(cell.language).equal('scala'); }); + it('Should keep cell language as python if cell has language override', async function (): Promise { + let cellData: nb.ICellContents = { + cell_type: CellTypes.Code, + source: 'print(\'1\')', + metadata: { language: 'python'}, + execution_count: 1 + }; + + let notebookModel = new NotebookModelStub({ + name: 'scala', + version: '', + mimetype: '' + }); + let cell = factory.createCell(cellData, { notebook: notebookModel, isTrusted: false }); + should(cell.language).equal('python'); + }); it('Should set cell language to python if no language defined', async function (): Promise { let cellData: nb.ICellContents = { @@ -127,7 +143,7 @@ describe('Cell Model', function (): void { let cellData: nb.ICellContents = { cell_type: CellTypes.Code, source: 'std::cout << "hello world";', - metadata: { language: 'python'}, + metadata: { }, execution_count: 1 }; @@ -144,7 +160,7 @@ describe('Cell Model', function (): void { let cellData: nb.ICellContents = { cell_type: CellTypes.Code, source: 'print(\'1\')', - metadata: { language: 'python'}, + metadata: { }, execution_count: 1 };