From fcec6905466e135fbe96788a7d69b9dc64822fb6 Mon Sep 17 00:00:00 2001 From: Vasu Bhog Date: Tue, 20 Oct 2020 13:26:59 -0500 Subject: [PATCH] Backend work for Notebook Parameterization and Tests (#12914) * Backend work for Parameterization + Tests * minor comments * fix test * address comments --- .../notebooks/common/outputRegistry.ts | 2 + .../notebook/browser/cellToolbarActions.ts | 40 +++++ .../test/browser/cellToolbarActions.test.ts | 2 +- .../test/electron-browser/cell.test.ts | 162 +++++++++++++++++- .../services/notebook/browser/models/cell.ts | 83 ++++++++- .../browser/models/modelInterfaces.ts | 3 + 6 files changed, 286 insertions(+), 6 deletions(-) diff --git a/src/sql/platform/notebooks/common/outputRegistry.ts b/src/sql/platform/notebooks/common/outputRegistry.ts index a566116e53..bb172e1455 100644 --- a/src/sql/platform/notebooks/common/outputRegistry.ts +++ b/src/sql/platform/notebooks/common/outputRegistry.ts @@ -10,6 +10,8 @@ export const Extensions = { }; export const HideInputTag = 'hide_input'; +export const ParametersTag = 'parameters'; +export const InjectedParametersTag = 'injected-parameters'; export interface ICellComponentRegistry { registerComponent(component: any): void; diff --git a/src/sql/workbench/contrib/notebook/browser/cellToolbarActions.ts b/src/sql/workbench/contrib/notebook/browser/cellToolbarActions.ts index 04b601c3d4..5732a8cb7d 100644 --- a/src/sql/workbench/contrib/notebook/browser/cellToolbarActions.ts +++ b/src/sql/workbench/contrib/notebook/browser/cellToolbarActions.ts @@ -145,6 +145,9 @@ export class CellToggleMoreActions { instantiationService.createInstance(CollapseCellAction, 'collapseCell', localize('collapseCell', "Collapse Cell"), true), instantiationService.createInstance(CollapseCellAction, 'expandCell', localize('expandCell', "Expand Cell"), false), new Separator(), + instantiationService.createInstance(ParametersCellAction, 'makeParameterCell', localize('makeParameterCell', "Make parameter cell"), true), + instantiationService.createInstance(ParametersCellAction, 'removeParameterCell', localize('RemoveParameterCell', "Remove parameter cell"), false), + new Separator(), instantiationService.createInstance(ClearCellOutputAction, 'clear', localize('clear', "Clear Result")), ); } @@ -364,3 +367,40 @@ export class ToggleMoreActions extends Action { return Promise.resolve(true); } } + +export class ParametersCellAction extends CellActionBase { + constructor(id: string, + label: string, + private parametersCell: boolean, + @INotificationService notificationService: INotificationService + ) { + super(id, label, undefined, notificationService); + } + + public canRun(context: CellContext): boolean { + return context.cell?.cellType === CellTypes.Code; + } + + async doRun(context: CellContext): Promise { + try { + let cell = context.cell || context.model.activeCell; + if (cell) { + if (this.parametersCell) { + if (!cell.isParameter) { + cell.isParameter = true; + } + } else { + if (cell.isParameter) { + cell.isParameter = false; + } + } + } + } catch (error) { + let message = getErrorMessage(error); + this.notificationService.notify({ + severity: Severity.Error, + message: message + }); + } + } +} diff --git a/src/sql/workbench/contrib/notebook/test/browser/cellToolbarActions.test.ts b/src/sql/workbench/contrib/notebook/test/browser/cellToolbarActions.test.ts index 5b87693eb5..e2b06838cb 100644 --- a/src/sql/workbench/contrib/notebook/test/browser/cellToolbarActions.test.ts +++ b/src/sql/workbench/contrib/notebook/test/browser/cellToolbarActions.test.ts @@ -125,7 +125,7 @@ suite('CellToolbarActions', function (): void { cellModelMock.setup(x => x.cellType).returns(() => 'code'); const action = new CellToggleMoreActions(instantiationService); action.onInit(testContainer, contextMock.object); - assert.equal(action['_moreActions']['viewItems'][0]['_action']['_actions'].length, 15, 'Unexpected number of valid elements'); + assert.equal(action['_moreActions']['viewItems'][0]['_action']['_actions'].length, 18, 'Unexpected number of valid elements'); }); test('CellToggleMoreActions with Markdown CellType', function (): void { diff --git a/src/sql/workbench/contrib/notebook/test/electron-browser/cell.test.ts b/src/sql/workbench/contrib/notebook/test/electron-browser/cell.test.ts index 6c3a9332ee..ad0377f28b 100644 --- a/src/sql/workbench/contrib/notebook/test/electron-browser/cell.test.ts +++ b/src/sql/workbench/contrib/notebook/test/electron-browser/cell.test.ts @@ -340,7 +340,7 @@ suite('Cell Model', function (): void { assert(!isCollapsed); }); - test('Should not allow markdown cells to be collapsible', async function (): Promise { + test('Should not allow markdown cells to be collapsible or parameters', async function (): Promise { let mdCellData: nb.ICellContents = { cell_type: CellTypes.Markdown, source: 'some *markdown*', @@ -349,11 +349,16 @@ suite('Cell Model', function (): void { }; let cell = factory.createCell(mdCellData, undefined); assert(cell.isCollapsed === false); + assert(cell.isParameter === false); + cell.isCollapsed = true; - // The typescript compiler will complain if we don't ignore the error from the following line, - // claiming that cell.isCollapsed will return true. It doesn't. + cell.isParameter = true; + // The typescript compiler will complain if we don't ignore the error from the following lines, + // claiming that cell.isCollapsed and cell.isParameter will return true. It doesn't. // @ts-ignore assert(cell.isCollapsed === false); + // @ts-ignore + assert(cell.isParameter === false); let codeCellData: nb.ICellContents = { cell_type: CellTypes.Code, @@ -364,8 +369,159 @@ suite('Cell Model', function (): void { }; cell = factory.createCell(codeCellData, undefined); assert(cell.isCollapsed === false); + assert(cell.isParameter === false); + cell.isCollapsed = true; assert(cell.isCollapsed === true); + cell.isParameter = true; + assert(cell.isParameter === true); + }); + + test('Should parse metadata\'s parameters tag correctly', async function (): Promise { + // Setup Notebook Model and Contents + let notebookModel = new NotebookModelStub({ + name: '', + version: '', + mimetype: '' + }); + let contents: nb.ICellContents = { + cell_type: CellTypes.Code, + source: '' + }; + let model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + + assert(!model.isParameter); + model.isParameter = true; + assert(model.isParameter); + model.isParameter = false; + assert(!model.isParameter); + + // Should not have parameters cell + let modelJson = model.toJSON(); + assert(!isUndefinedOrNull(modelJson.metadata.tags)); + assert(!modelJson.metadata.tags.some(x => x === 'parameter')); + + // Add parameters tag + contents.metadata = { + tags: ['parameters'] + }; + model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + + assert(model.isParameter); + model.isParameter = false; + assert(!model.isParameter); + model.isParameter = true; + assert(model.isParameter); + + // Should find parameters tag in metadata + modelJson = model.toJSON(); + assert(!isUndefinedOrNull(modelJson.metadata.tags)); + assert(modelJson.metadata.tags.some(x => x === 'parameters')); + + contents.metadata = { + tags: ['not_a_real_tag'] + }; + model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + modelJson = model.toJSON(); + assert(!isUndefinedOrNull(modelJson.metadata.tags)); + assert(!modelJson.metadata.tags.some(x => x === 'parameters')); + + contents.metadata = { + tags: ['not_a_real_tag', 'parameters'] + }; + model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + modelJson = model.toJSON(); + assert(!isUndefinedOrNull(modelJson.metadata.tags)); + assert(modelJson.metadata.tags.some(x => x === 'parameters')); + }); + + test('Should emit event setting cell as Parameter', async function (): Promise { + let notebookModel = new NotebookModelStub({ + name: '', + version: '', + mimetype: '' + }); + let contents: nb.ICellContents = { + cell_type: CellTypes.Code, + source: '' + }; + + let model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + assert(!model.isParameter); + + let createParameterPromise = () => { + return new Promise((resolve, reject) => { + setTimeout(() => reject(), 2000); + model.onParameterStateChanged(isParameter => { + resolve(isParameter); + }); + }); + }; + + assert(!model.isParameter); + let parameterPromise = createParameterPromise(); + model.isParameter = true; + let isParameter = await parameterPromise; + assert(isParameter); + + parameterPromise = createParameterPromise(); + model.isParameter = false; + isParameter = await parameterPromise; + assert(!isParameter); + }); + + test('Should parse metadata\'s injected parameter tag correctly', async function (): Promise { + let notebookModel = new NotebookModelStub({ + name: '', + version: '', + mimetype: '' + }); + let contents: nb.ICellContents = { + cell_type: CellTypes.Code, + source: '' + }; + let model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + + assert(!model.isInjectedParameter); + model.isInjectedParameter = true; + assert(model.isInjectedParameter); + model.isInjectedParameter = false; + assert(!model.isInjectedParameter); + + let modelJson = model.toJSON(); + assert(!isUndefinedOrNull(modelJson.metadata.tags)); + assert(!modelJson.metadata.tags.some(x => x === 'injected-parameters')); + + contents.metadata = { + tags: ['injected-parameters'] + }; + model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + + assert(model.isInjectedParameter); + model.isInjectedParameter = false; + assert(!model.isInjectedParameter); + model.isInjectedParameter = true; + assert(model.isInjectedParameter); + + modelJson = model.toJSON(); + assert(!isUndefinedOrNull(modelJson.metadata.tags)); + assert(modelJson.metadata.tags.some(x => x === 'injected-parameters')); + + contents.metadata = { + tags: ['not_a_real_tag'] + }; + model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + modelJson = model.toJSON(); + assert(!isUndefinedOrNull(modelJson.metadata.tags)); + assert(!modelJson.metadata.tags.some(x => x === 'injected-parameters')); + + contents.metadata = { + tags: ['not_a_real_tag', 'injected-parameters'] + }; + model = factory.createCell(contents, { notebook: notebookModel, isTrusted: false }); + modelJson = model.toJSON(); + assert(!isUndefinedOrNull(modelJson.metadata.tags)); + assert(modelJson.metadata.tags.some(x => x === 'injected-parameters')); }); suite('Model Future handling', function (): void { diff --git a/src/sql/workbench/services/notebook/browser/models/cell.ts b/src/sql/workbench/services/notebook/browser/models/cell.ts index 302c634848..28b7393be4 100644 --- a/src/sql/workbench/services/notebook/browser/models/cell.ts +++ b/src/sql/workbench/services/notebook/browser/models/cell.ts @@ -22,7 +22,7 @@ import { optional } from 'vs/platform/instantiation/common/instantiation'; import { getErrorMessage, onUnexpectedError } from 'vs/base/common/errors'; import { generateUuid } from 'vs/base/common/uuid'; import { IModelContentChangedEvent } from 'vs/editor/common/model/textModelEvents'; -import { HideInputTag } from 'sql/platform/notebooks/common/outputRegistry'; +import { HideInputTag, ParametersTag, InjectedParametersTag } from 'sql/platform/notebooks/common/outputRegistry'; import { FutureInternal, notebookConstants } from 'sql/workbench/services/notebook/browser/interfaces'; import { ICommandService } from 'vs/platform/commands/common/commands'; import { tryMatchCellMagic, extractCellMagicCommandPlusArgs } from 'sql/workbench/services/notebook/browser/utils'; @@ -68,6 +68,9 @@ export class CellModel extends Disposable implements ICellModel { private _cellSourceChanged: boolean = false; private _gridDataConversionComplete: Promise[] = []; private _defaultToWYSIWYG: boolean; + private _isParameter: boolean; + private _onParameterStateChanged = new Emitter(); + private _isInjectedParameter: boolean; constructor(cellData: nb.ICellContents, private _options: ICellModelOptions, @@ -340,6 +343,78 @@ export class CellModel extends Disposable implements ICellModel { return this._onCellMarkdownChanged.event; } + public get onParameterStateChanged(): Event { + return this._onParameterStateChanged.event; + } + + public get isParameter() { + return this._isParameter; + } + + public set isParameter(value: boolean) { + if (this.cellType !== CellTypes.Code) { + return; + } + let stateChanged = this._isParameter !== value; + this._isParameter = value; + + let tagIndex = -1; + if (this._metadata.tags) { + tagIndex = this._metadata.tags.findIndex(tag => tag === ParametersTag); + } + + if (this._isParameter) { + if (tagIndex === -1) { + if (!this._metadata.tags) { + this._metadata.tags = []; + } + this._metadata.tags.push(ParametersTag); + } + } else { + if (tagIndex > -1) { + this._metadata.tags.splice(tagIndex, 1); + } + } + + if (stateChanged) { + this._onParameterStateChanged.fire(this._isParameter); + this.sendChangeToNotebook(NotebookChangeType.CellInputVisibilityChanged); + } + } + + /** + Injected Parameters will be used for future scenarios + when we need to hide this cell for Parameterization + */ + public get isInjectedParameter() { + return this._isInjectedParameter; + } + + public set isInjectedParameter(value: boolean) { + if (this.cellType !== CellTypes.Code) { + return; + } + this._isInjectedParameter = value; + + let tagIndex = -1; + if (this._metadata.tags) { + tagIndex = this._metadata.tags.findIndex(tag => tag === InjectedParametersTag); + } + + if (this._isInjectedParameter) { + if (tagIndex === -1) { + if (!this._metadata.tags) { + this._metadata.tags = []; + } + this._metadata.tags.push(InjectedParametersTag); + } + } else { + if (tagIndex > -1) { + this._metadata.tags.splice(tagIndex, 1); + } + } + } + private notifyExecutionComplete(): void { if (this._notebookService) { this._notebookService.serializeNotebookStateChange(this.notebookModel.notebookUri, NotebookChangeType.CellExecuted, this) @@ -727,10 +802,14 @@ export class CellModel extends Disposable implements ICellModel { this._source = this.getMultilineSource(cell.source); this._metadata = cell.metadata || {}; - if (this._metadata.tags && this._metadata.tags.some(x => x === HideInputTag) && this._cellType === CellTypes.Code) { + if (this._metadata.tags && this._metadata.tags.some(x => x === HideInputTag || x === ParametersTag || x === InjectedParametersTag) && this._cellType === CellTypes.Code) { this._isCollapsed = true; + this._isParameter = true; + this._isInjectedParameter = true; } else { this._isCollapsed = false; + this._isParameter = false; + this._isInjectedParameter = false; } this._cellGuid = cell.metadata && cell.metadata.azdata_cell_guid ? cell.metadata.azdata_cell_guid : generateUuid(); diff --git a/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts b/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts index 1ebd233fef..d8a30b743c 100644 --- a/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts +++ b/src/sql/workbench/services/notebook/browser/models/modelInterfaces.ts @@ -477,7 +477,10 @@ export interface ICellModel { stdInVisible: boolean; readonly onLoaded: Event; isCollapsed: boolean; + isParameter: boolean; + isInjectedParameter: boolean; readonly onCollapseStateChanged: Event; + readonly onParameterStateChanged: Event; readonly onCellModeChanged: Event; modelContentChangedEvent: IModelContentChangedEvent; isEditMode: boolean;