Fix converting from a python script (#8699)

* Fix converting from a python script

* Fix linter error

Author: rchiodo

news/2 Fixes/8677.md

@@ -0,0 +1 @@
+Converting to python script no longer working from a notebook.

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

@@ -791,7 +791,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
             tempFile = await this.fileSystem.createTemporaryFile('.ipynb');
 
             // Translate the cells into a notebook
-            await this.fileSystem.writeFile(tempFile.filePath, this.generateNotebookContent(cells), { encoding: 'utf-8' });
+            await this.fileSystem.writeFile(tempFile.filePath, await this.generateNotebookContent(cells), { encoding: 'utf-8' });
 
             // Import this file and show it
             const contents = await this.importer.importFromFile(tempFile.filePath, this.file.fsPath);

src/client/datascience/jupyter/jupyterImporter.ts

@@ -1,14 +1,16 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 'use strict';
+import '../../common/extensions';
+
 import { nbformat } from '@jupyterlab/coreutils';
 import * as fs from 'fs-extra';
 import { inject, injectable } from 'inversify';
 import * as os from 'os';
 import * as path from 'path';
-import '../../common/extensions';
 
 import { IWorkspaceService } from '../../common/application/types';
+import { traceError } from '../../common/logger';
 import { IFileSystem, IPlatformService } from '../../common/platform/types';
 import { IConfigurationService, IDisposableRegistry } from '../../common/types';
 import * as localize from '../../common/utils/localize';
@@ -138,35 +140,39 @@ export class JupyterImporter implements INotebookImporter {
     // When importing a file, calculate if we can create a %cd so that the relative paths work
     private async calculateDirectoryChange(notebookFile: string): Promise<string | undefined> {
         let directoryChange: string | undefined;
-        // Make sure we don't already have an import/export comment in the file
-        const contents = await this.fileSystem.readFile(notebookFile);
-        const haveChangeAlready = contents.includes(CodeSnippits.ChangeDirectoryCommentIdentifier);
-
-        if (!haveChangeAlready) {
-            const notebookFilePath = path.dirname(notebookFile);
-            // First see if we have a workspace open, this only works if we have a workspace root to be relative to
-            if (this.workspaceService.hasWorkspaceFolders) {
-                const workspacePath = this.workspaceService.workspaceFolders![0].uri.fsPath;
-
-                // Make sure that we have everything that we need here
-                if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) {
-                    directoryChange = path.relative(workspacePath, notebookFilePath);
+        try {
+            // Make sure we don't already have an import/export comment in the file
+            const contents = await this.fileSystem.readFile(notebookFile);
+            const haveChangeAlready = contents.includes(CodeSnippits.ChangeDirectoryCommentIdentifier);
+
+            if (!haveChangeAlready) {
+                const notebookFilePath = path.dirname(notebookFile);
+                // First see if we have a workspace open, this only works if we have a workspace root to be relative to
+                if (this.workspaceService.hasWorkspaceFolders) {
+                    const workspacePath = this.workspaceService.workspaceFolders![0].uri.fsPath;
+
+                    // Make sure that we have everything that we need here
+                    if (workspacePath && path.isAbsolute(workspacePath) && notebookFilePath && path.isAbsolute(notebookFilePath)) {
+                        directoryChange = path.relative(workspacePath, notebookFilePath);
+                    }
                 }
             }
-        }
 
-        // If path.relative can't calculate a relative path, then it just returns the full second path
-        // so check here, we only want this if we were able to calculate a relative path, no network shares or drives
-        if (directoryChange && !path.isAbsolute(directoryChange)) {
+            // If path.relative can't calculate a relative path, then it just returns the full second path
+            // so check here, we only want this if we were able to calculate a relative path, no network shares or drives
+            if (directoryChange && !path.isAbsolute(directoryChange)) {
 
-            // Escape windows path chars so they end up in the source escaped
-            if (this.platform.isWindows) {
-                directoryChange = directoryChange.replace('\\', '\\\\');
-            }
+                // Escape windows path chars so they end up in the source escaped
+                if (this.platform.isWindows) {
+                    directoryChange = directoryChange.replace('\\', '\\\\');
+                }
 
-            return directoryChange;
-        } else {
-            return undefined;
+                return directoryChange;
+            } else {
+                return undefined;
+            }
+        } catch (e) {
+            traceError(e);
         }
     }
 

src/test/datascience/liveshare.functional.test.tsx

@@ -25,7 +25,7 @@ import {
     IJupyterExecution
 } from '../../client/datascience/types';
 import { InteractivePanel } from '../../datascience-ui/history-react/interactivePanel';
-import { asyncDump } from '../common/asyncDump';
+//import { asyncDump } from '../common/asyncDump';
 import { DataScienceIocContainer } from './dataScienceIocContainer';
 import { createDocument } from './editor-integration/helpers';
 import { waitForUpdate } from './reactHelpers';
@@ -62,7 +62,7 @@ suite('DataScience LiveShare tests', () => {
     });
 
     suiteTeardown(() => {
-        asyncDump();
+        //asyncDump();
     });
 
     function createContainer(role: vsls.Role): DataScienceIocContainer {

src/test/datascience/mockJupyterManager.ts

@@ -401,7 +401,7 @@ export class MockJupyterManager implements IJupyterSessionManager {
             this.setupPythonServiceExec(service, 'jupyter', ['nbconvert', '--version'], () => Promise.resolve({ stdout: '1.1.1.1' }));
             this.setupPythonServiceExec(service, 'jupyter', ['nbconvert', /.*/, '--to', 'python', '--stdout', '--template', /.*/], () => {
                 return Promise.resolve({
-                    stdout: '#%%\r\nimport os\r\nos.chdir()'
+                    stdout: '#%%\r\nimport os\r\nos.chdir()\r\n#%%\r\na=1'
                 });
             });
         }

src/test/datascience/nativeEditor.functional.test.tsx

@@ -226,17 +226,17 @@ for _ in range(50):
             assert.equal(afterDelete.length, 1, `Delete should NOT remove the last cell`);
         }, () => { return ioc; });
 
-        runMountedTest('Export', async (wrapper) => {
+        runMountedTest('Convert to python', async (wrapper) => {
             // Export should cause the export dialog to come up. Remap appshell so we can check
             const dummyDisposable = {
                 dispose: () => { return; }
             };
-            let exportCalled = false;
+            let saveCalled = false;
             const appShell = TypeMoq.Mock.ofType<IApplicationShell>();
             appShell.setup(a => a.showErrorMessage(TypeMoq.It.isAnyString())).returns((e) => { throw e; });
             appShell.setup(a => a.showInformationMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(''));
             appShell.setup(a => a.showSaveDialog(TypeMoq.It.isAny())).returns(() => {
-                exportCalled = true;
+                saveCalled = true;
                 return Promise.resolve(undefined);
             });
             appShell.setup(a => a.setStatusBarMessage(TypeMoq.It.isAny())).returns(() => dummyDisposable);
@@ -247,9 +247,22 @@ for _ in range(50):
             await addCell(wrapper, ioc, 'a=1\na');
 
             // Export should cause exportCalled to change to true
-            const exportButton = findButton(wrapper, NativeEditor, 8);
+            const saveButton = findButton(wrapper, NativeEditor, 8);
+            await waitForMessageResponse(ioc, () => saveButton!.simulate('click'));
+            assert.equal(saveCalled, true, 'Save should have been called');
+
+            // Click export and wait for a document to change
+            const documentChange = createDeferred();
+            const docManager = ioc.get<IDocumentManager>(IDocumentManager) as MockDocumentManager;
+            docManager.onDidChangeTextDocument(() => documentChange.resolve());
+            const exportButton = findButton(wrapper, NativeEditor, 9);
             await waitForMessageResponse(ioc, () => exportButton!.simulate('click'));
-            assert.equal(exportCalled, true, 'Export should have been called');
+            await waitForPromise(documentChange.promise, 3000);
+            // Verify the new document is valid python
+            const newDoc = docManager.activeTextEditor;
+            assert.ok(newDoc, 'New doc not created');
+            assert.ok(newDoc!.document.getText().includes('a=1'), 'Export did not create a python file');
+
         }, () => { return ioc; });
 
         runMountedTest('RunAllCells', async (wrapper) => {