Minimize the changes to ipynb files when saving (microsoft#7982)

* Minimize the changes to ipynb files when saving * Use common location for source splitting * Make sure to throw json error before setting any state * Fix functional and unit tests * Fix linter problem

Author: rchiodo

news/2 Fixes/7960.md

@@ -0,0 +1 @@
+Minimize the changes to an ipynb file when saving - preserve metadata and spacing.

package-lock.json

@@ -2671,12 +2671,13 @@
  "verifyBundle": "gulp verifyBundle"
  },
  "dependencies": {
+ "@blueprintjs/select": "3.10.0",
  "@jupyterlab/services": "^3.2.1",
  "@msrvida/python-program-analysis": "^0.2.6",
- "@blueprintjs/select": "3.10.0",
  "ansi-regex": "^4.1.0",
  "arch": "^2.1.0",
  "azure-storage": "^2.10.3",
+ "detect-indent": "^6.0.0",
  "diff-match-patch": "^1.0.0",
  "fast-deep-equal": "^2.0.1",
  "fs-extra": "^4.0.3",

package.json

@@ -43,8 +43,7 @@ function generateCodeCell(code: string[], file: string, line: number, id: string
  id: id,
  file: file,
  line: line,
- state: CellState.init,
- type: 'execute'
+ state: CellState.init
  };
 
 }
@@ -55,7 +54,6 @@ function generateMarkdownCell(code: string[], file: string, line: number, id: st
  file: file,
  line: line,
  state: CellState.finished,
- type: 'execute',
  data: {
  cell_type: 'markdown',
  source: generateMarkdownFromCodeLines(code),

src/client/datascience/cellFactory.ts

@@ -7,7 +7,8 @@ import { noop } from '../../test/core';
 
 const SingleQuoteMultiline = '\'\'\'';
 const DoubleQuoteMultiline = '\"\"\"';
-export function concatMultilineString(str: nbformat.MultilineString): string {
+
+function concatMultilineString(str: nbformat.MultilineString, trim: boolean): string {
  const nonLineFeedWhiteSpaceTrim = /(^[\t\f\v\r ]+|[\t\f\v\r ]+$)/g; // Local var so don't have to reset the lastIndex.
  if (Array.isArray(str)) {
  let result = '';
@@ -21,16 +22,35 @@ export function concatMultilineString(str: nbformat.MultilineString): string {
  }
 
  // Just trim whitespace. Leave \n in place
- return result.replace(nonLineFeedWhiteSpaceTrim, '');
+ return trim ? result.replace(nonLineFeedWhiteSpaceTrim, '') : result;
  }
- return str.toString().replace(nonLineFeedWhiteSpaceTrim, '');
+ return trim ? str.toString().replace(nonLineFeedWhiteSpaceTrim, '') : str.toString();
 }
 
-export function splitMultilineString(str: nbformat.MultilineString): string[] {
- if (Array.isArray(str)) {
- return str as string[];
+export function concatMultilineStringOutput(str: nbformat.MultilineString): string {
+ return concatMultilineString(str, true);
+}
+export function concatMultilineStringInput(str: nbformat.MultilineString): string {
+ return concatMultilineString(str, false);
+}
+
+export function splitMultilineString(source: nbformat.MultilineString): string[] {
+ // Make sure a multiline string is back the way Jupyter expects it
+ if (Array.isArray(source)) {
+ return source as string[];
+ }
+ const str = source.toString();
+ if (str.length > 0) {
+ // Each line should be a separate entry, but end with a \n if not last entry
+ const arr = str.split('\n');
+ return arr.map((s, i) => {
+ if (i < arr.length - 1) {
+ return `${s}\n`;
+ }
+ return s;
+ }).filter(s => s.length > 0); // Skip last one if empty (it's the only one that could be length 0)
  }
- return str.toString().split('\n');
+ return [];
 }
 
 // Strip out comment lines from code

src/client/datascience/common.ts

@@ -14,7 +14,7 @@ import * as localize from '../../common/utils/localize';
 import { Common } from '../../common/utils/localize';
 import { noop } from '../../common/utils/misc';
 import { CellMatcher } from '../cellMatcher';
-import { concatMultilineString } from '../common';
+import { concatMultilineStringInput } from '../common';
 import { Identifiers } from '../constants';
 import { CellState, ICell as IVscCell, IGatherExecution, INotebookExecutionLogger } from '../types';
 
@@ -60,7 +60,7 @@ export class GatherExecution implements IGatherExecution, INotebookExecutionLogg
 
  // Strip first line marker. We can't do this at JupyterServer.executeCodeObservable because it messes up hashing
  const cellMatcher = new CellMatcher(this.configService.getSettings().datascience);
- cloneCell.data.source = cellMatcher.stripFirstMarker(concatMultilineString(vscCell.data.source));
+ cloneCell.data.source = cellMatcher.stripFirstMarker(concatMultilineStringInput(vscCell.data.source));
 
  // Convert IVscCell to IGatherCell
  const cell = convertVscToGatherCell(cloneCell) as LogCell;

src/client/datascience/gather/gather.ts

@@ -22,7 +22,7 @@ import { CancellationError } from '../../../common/cancellation';
 import { traceWarning } from '../../../common/logger';
 import { IFileSystem, TemporaryFile } from '../../../common/platform/types';
 import { createDeferred, Deferred, waitForPromise } from '../../../common/utils/async';
-import { concatMultilineString } from '../../common';
+import { concatMultilineStringInput } from '../../common';
 import { Identifiers, Settings } from '../../constants';
 import { IInteractiveWindowListener, IInteractiveWindowProvider, IJupyterExecution, INotebook } from '../../types';
 import {
@@ -387,7 +387,7 @@ export abstract class BaseIntellisenseProvider implements IInteractiveWindowList
  if (document) {
  const changes = document.loadAllCells(payload.cells.filter(c => c.data.cell_type === 'code').map(cell => {
  return {
- code: concatMultilineString(cell.data.source),
+ code: concatMultilineStringInput(cell.data.source),
  id: cell.id
  };
  }));

src/client/datascience/interactive-common/intellisense/baseIntellisenseProvider.ts

@@ -557,13 +557,12 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
  return result;
  }
 
- protected addMessageImpl(message: string, type: 'preview' | 'execute'): void {
+ protected addMessageImpl(message: string): void {
  const cell: ICell = {
  id: uuid(),
  file: Identifiers.EmptyFileName,
  line: 0,
  state: CellState.finished,
- type,
  data: {
  cell_type: 'messages',
  messages: [message],

src/client/datascience/interactive-common/interactiveBase.ts

@@ -3,13 +3,22 @@
 'use strict';
 import '../../common/extensions';
 
+import { nbformat } from '@jupyterlab/coreutils/lib/nbformat';
+import * as detectIndent from 'detect-indent';
 import * as fastDeepEqual from 'fast-deep-equal';
 import { inject, injectable, multiInject, named } from 'inversify';
 import * as path from 'path';
 import * as uuid from 'uuid/v4';
 import { Event, EventEmitter, Memento, Uri, ViewColumn } from 'vscode';
 
-import { IApplicationShell, ICommandManager, IDocumentManager, ILiveShareApi, IWebPanelProvider, IWorkspaceService } from '../../common/application/types';
+import {
+ IApplicationShell,
+ ICommandManager,
+ IDocumentManager,
+ ILiveShareApi,
+ IWebPanelProvider,
+ IWorkspaceService
+} from '../../common/application/types';
 import { ContextKey } from '../../common/contextKey';
 import { traceError } from '../../common/logger';
 import { IFileSystem, TemporaryFile } from '../../common/platform/types';
@@ -20,10 +29,23 @@ import { StopWatch } from '../../common/utils/stopWatch';
 import { EXTENSION_ROOT_DIR } from '../../constants';
 import { IInterpreterService } from '../../interpreter/contracts';
 import { captureTelemetry, sendTelemetryEvent } from '../../telemetry';
-import { concatMultilineString } from '../common';
-import { EditorContexts, Identifiers, NativeKeyboardCommandTelemetryLookup, NativeMouseCommandTelemetryLookup, Telemetry } from '../constants';
+import { concatMultilineStringInput, splitMultilineString } from '../common';
+import {
+ EditorContexts,
+ Identifiers,
+ NativeKeyboardCommandTelemetryLookup,
+ NativeMouseCommandTelemetryLookup,
+ Telemetry
+} from '../constants';
 import { InteractiveBase } from '../interactive-common/interactiveBase';
-import { IEditCell, INativeCommand, InteractiveWindowMessages, ISaveAll, ISubmitNewCell } from '../interactive-common/interactiveWindowTypes';
+import {
+ IEditCell,
+ INativeCommand,
+ InteractiveWindowMessages,
+ ISaveAll,
+ ISubmitNewCell
+} from '../interactive-common/interactiveWindowTypes';
+import { InvalidNotebookFileError } from '../jupyter/invalidNotebookFileError';
 import {
  CellState,
  ICell,
@@ -62,6 +84,8 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  private visibleCells: ICell[] = [];
  private startupTimer: StopWatch = new StopWatch();
  private loadedAllCells: boolean = false;
+ private indentAmount: string = ' ';
+ private notebookJson: Partial<nbformat.INotebookContent> = {};
 
  constructor(
  @multiInject(IInteractiveWindowListener) listeners: IInteractiveWindowListener[],
@@ -132,7 +156,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  this.close().ignoreErrors();
  }
 
- public async load(content: string, file: Uri): Promise<void> {
+ public async load(contents: string, file: Uri): Promise<void> {
  // Save our uri
  this._file = file;
 
@@ -149,12 +173,10 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  const dirtyContents = this.getStoredContents();
  if (dirtyContents) {
  // This means we're dirty. Indicate dirty and load from this content
- const cells = await this.importer.importCells(dirtyContents);
- return this.loadCells(cells, true);
+ return this.loadContents(dirtyContents, true);
  } else {
- // Load the contents of this notebook into our cells.
- const cells = content ? await this.importer.importCells(content) : [];
- return this.loadCells(cells, false);
+ // Load without setting dirty
+ return this.loadContents(contents, false);
  }
  }
 
@@ -318,8 +340,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  id: info.id,
  file: Identifiers.EmptyFileName,
  line: 0,
- state: CellState.error,
- type: 'execute'
+ state: CellState.error
  }
  ]);
 
@@ -391,6 +412,41 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  // Actually don't close, just let the error bubble out
  }
 
+ private async loadContents(contents: string | undefined, forceDirty: boolean): Promise<void> {
+ // tslint:disable-next-line: no-any
+ const json = contents ? JSON.parse(contents) as any : undefined;
+
+ // Double check json (if we have any)
+ if (json && !json.cells) {
+ throw new InvalidNotebookFileError(this.file.fsPath);
+ }
+
+ // Then compute indent. It's computed from the contents
+ if (contents) {
+ this.indentAmount = detectIndent(contents).indent;
+ }
+
+ // Then save the contents. We'll stick our cells back into this format when we save
+ if (json) {
+ this.notebookJson = json;
+ }
+
+ // Extract cells from the json
+ const cells = contents ? json.cells as (nbformat.ICodeCell | nbformat.IRawCell | nbformat.IMarkdownCell)[] : [];
+
+ // Then parse the cells
+ return this.loadCells(cells.map((c, index) => {
+ return {
+ id: `NotebookImport#${index}`,
+ file: Identifiers.EmptyFileName,
+ line: 0,
+ state: CellState.finished,
+ data: c
+ };
+ }), forceDirty);
+
+ }
+
  private async loadCells(cells: ICell[], forceDirty: boolean): Promise<void> {
  // Make sure cells have at least 1
  if (cells.length === 0) {
@@ -399,7 +455,6 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  line: 0,
  file: Identifiers.EmptyFileName,
  state: CellState.finished,
- type: 'execute',
  data: {
  cell_type: 'code',
  outputs: [],
@@ -419,7 +474,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  if (forceDirty) {
  await this.setDirty();
  }
- sendTelemetryEvent(Telemetry.CellCount, undefined, {count: cells.length});
+ sendTelemetryEvent(Telemetry.CellCount, undefined, { count: cells.length });
  return this.postMessage(InteractiveWindowMessages.LoadAllCells, { cells });
  }
 
@@ -497,7 +552,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  const cell = this.visibleCells.find(c => c.id === request.id);
  if (cell) {
  // This is an actual edit.
- const contents = concatMultilineString(cell.data.source);
+ const contents = concatMultilineStringInput(cell.data.source);
  const before = contents.substr(0, change.rangeOffset);
  const after = contents.substr(change.rangeOffset + change.rangeLength);
  const newContents = `${before}${normalized}${after}`;
@@ -533,8 +588,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  this.visibleCells = cells;
 
  // Save our dirty state in the storage for reopen later
- const notebook = await this.jupyterExporter.translateToNotebook(this.visibleCells, undefined);
- await this.storeContents(JSON.stringify(notebook));
+ await this.storeContents(this.generateNotebookContent(cells));
 
  // Indicate dirty
  await this.setDirty();
@@ -569,10 +623,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  tempFile = await this.fileSystem.createTemporaryFile('.ipynb');
 
  // Translate the cells into a notebook
- const notebook = await this.jupyterExporter.translateToNotebook(cells, undefined);
-
- // Write the cells to this file
- await this.fileSystem.writeFile(tempFile.filePath, JSON.stringify(notebook), { encoding: 'utf-8' });
+ await this.fileSystem.writeFile(tempFile.filePath, this.generateNotebookContent(cells), { encoding: 'utf-8' });
 
  // Import this file and show it
  const contents = await this.importer.importFromFile(tempFile.filePath);
@@ -594,6 +645,23 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  await this.documentManager.showTextDocument(doc, ViewColumn.One);
  }
 
+ private fixupCell(cell: nbformat.ICell): nbformat.ICell {
+ // Source is usually a single string on input. Convert back to an array
+ return {
+ ...cell,
+ source: splitMultilineString(cell.source)
+ };
+ }
+
+ private generateNotebookContent(cells: ICell[]): string {
+ // Reuse our original json except for the cells.
+ const json = {
+ ...this.notebookJson,
+ cells: cells.map(c => this.fixupCell(c.data))
+ };
+ return JSON.stringify(json, null, this.indentAmount);
+ }
+
  @captureTelemetry(Telemetry.Save, undefined, true)
  private async saveToDisk(): Promise<void> {
  try {
@@ -617,9 +685,8 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
  }
 
  if (fileToSaveTo && isDirty) {
- // Save our visible cells into the file
- const notebook = await this.jupyterExporter.translateToNotebook(this.visibleCells, undefined);
- await this.fileSystem.writeFile(fileToSaveTo.fsPath, JSON.stringify(notebook));
+ // Write out our visible cells
+ await this.fileSystem.writeFile(fileToSaveTo.fsPath, this.generateNotebookContent(this.visibleCells));
 
  // Update our file name and dirty state
  this._file = fileToSaveTo;

src/client/datascience/interactive-ipynb/nativeEditor.ts

@@ -118,7 +118,7 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi
  }
 
  public addMessage(message: string): Promise<void> {
- this.addMessageImpl(message, 'execute');
+ this.addMessageImpl(message);
  return Promise.resolve();
  }