Skip to content

Commit 25f9bfb

Browse files
techwithtimtechwithtim
authored
Refactored export related features (#12953)
* did main refactor * started updating tests * added proper tests * continued fixing tests
1 parent 9d2d4d2 commit 25f9bfb

File tree

8 files changed

+156
-143
lines changed

8 files changed

+156
-143
lines changed

‎src/client/datascience/export/exportManagerDependencyChecker.ts renamed to ‎src/client/datascience/export/exportDependencyChecker.ts

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,19 @@
11
import { inject, injectable } from 'inversify';
2-
import { Uri } from 'vscode';
32
import * as localize from '../../common/utils/localize';
43
import { ProgressReporter } from '../progress/progressReporter';
5-
import { IJupyterExecution, IJupyterInterpreterDependencyManager, INotebookModel } from '../types';
6-
import { ExportManager } from './exportManager';
7-
import { ExportFormat, IExportManager } from './types';
4+
import { IJupyterExecution, IJupyterInterpreterDependencyManager } from '../types';
5+
import { ExportFormat } from './types';
86

97
@injectable()
10-
export class ExportManagerDependencyChecker implements IExportManager {
8+
export class ExportDependencyChecker {
119
constructor(
12-
@inject(ExportManager) private readonly manager: IExportManager,
1310
@inject(IJupyterExecution) private jupyterExecution: IJupyterExecution,
1411
@inject(IJupyterInterpreterDependencyManager)
1512
private readonly dependencyManager: IJupyterInterpreterDependencyManager,
1613
@inject(ProgressReporter) private readonly progressReporter: ProgressReporter
1714
) {}
1815

19-
public async export(
20-
format: ExportFormat,
21-
model: INotebookModel,
22-
defaultFileName?: string
23-
): Promise<Uri | undefined> {
16+
public async checkDependencies(format: ExportFormat) {
2417
// Before we try the import, see if we don't support it, if we don't give a chance to install dependencies
2518
const reporter = this.progressReporter.createProgressIndicator(`Exporting to ${format}`);
2619
try {
@@ -33,6 +26,5 @@ export class ExportManagerDependencyChecker implements IExportManager {
3326
} finally {
3427
reporter.dispose();
3528
}
36-
return this.manager.export(format, model, defaultFileName);
3729
}
3830
}

‎src/client/datascience/export/exportManagerFileOpener.ts renamed to ‎src/client/datascience/export/exportFileOpener.ts

Lines changed: 3 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -2,53 +2,23 @@ import { inject, injectable } from 'inversify';
22
import { Position, Uri } from 'vscode';
33
import { IApplicationShell, IDocumentManager } from '../../common/application/types';
44
import { PYTHON_LANGUAGE } from '../../common/constants';
5-
import { traceError } from '../../common/logger';
65
import { IFileSystem } from '../../common/platform/types';
76
import { IBrowserService } from '../../common/types';
87
import * as localize from '../../common/utils/localize';
98
import { sendTelemetryEvent } from '../../telemetry';
109
import { Telemetry } from '../constants';
11-
import { INotebookModel } from '../types';
12-
import { ExportManagerDependencyChecker } from './exportManagerDependencyChecker';
13-
import { ExportFormat, IExportManager } from './types';
10+
import { ExportFormat } from './types';
1411

1512
@injectable()
16-
export class ExportManagerFileOpener implements IExportManager {
13+
export class ExportFileOpener {
1714
constructor(
18-
@inject(ExportManagerDependencyChecker) private readonly manager: IExportManager,
1915
@inject(IDocumentManager) protected readonly documentManager: IDocumentManager,
2016
@inject(IFileSystem) private readonly fileSystem: IFileSystem,
2117
@inject(IApplicationShell) private readonly applicationShell: IApplicationShell,
2218
@inject(IBrowserService) private readonly browserService: IBrowserService
2319
) {}
2420

25-
public async export(
26-
format: ExportFormat,
27-
model: INotebookModel,
28-
defaultFileName?: string
29-
): Promise<Uri | undefined> {
30-
let uri: Uri | undefined;
31-
try {
32-
uri = await this.manager.export(format, model, defaultFileName);
33-
} catch (e) {
34-
let msg = e;
35-
traceError('Export failed', e);
36-
sendTelemetryEvent(Telemetry.ExportNotebookAsFailed, undefined, { format: format });
37-
38-
if (format === ExportFormat.pdf) {
39-
msg = localize.DataScience.exportToPDFDependencyMessage();
40-
}
41-
42-
this.showExportFailed(msg);
43-
return;
44-
}
45-
46-
if (!uri) {
47-
// if export didn't fail but no uri returned then user cancelled operation
48-
sendTelemetryEvent(Telemetry.ExportNotebookAs, undefined, { format: format, cancelled: true });
49-
return;
50-
}
51-
21+
public async openFile(format: ExportFormat, uri: Uri) {
5222
if (format === ExportFormat.python) {
5323
await this.openPythonFile(uri);
5424
sendTelemetryEvent(Telemetry.ExportNotebookAs, undefined, {
@@ -77,15 +47,6 @@ export class ExportManagerFileOpener implements IExportManager {
7747
});
7848
}
7949

80-
private showExportFailed(msg: string) {
81-
this.applicationShell
82-
.showErrorMessage(
83-
// tslint:disable-next-line: messages-must-be-localized
84-
`${localize.DataScience.failedExportMessage()} ${msg}`
85-
)
86-
.then();
87-
}
88-
8950
private async askOpenFile(uri: Uri): Promise<boolean> {
9051
const yes = localize.DataScience.openExportFileYes();
9152
const no = localize.DataScience.openExportFileNo();

‎src/client/datascience/export/exportManager.ts

Lines changed: 85 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,17 @@
11
import { inject, injectable, named } from 'inversify';
2+
import { CancellationToken } from 'monaco-editor';
23
import * as path from 'path';
34
import { Uri } from 'vscode';
4-
import { IFileSystem } from '../../common/platform/types';
5+
import { IApplicationShell } from '../../common/application/types';
6+
import { traceError } from '../../common/logger';
7+
import { IFileSystem, TemporaryDirectory } from '../../common/platform/types';
8+
import * as localize from '../../common/utils/localize';
9+
import { sendTelemetryEvent } from '../../telemetry';
10+
import { Telemetry } from '../constants';
511
import { ProgressReporter } from '../progress/progressReporter';
612
import { INotebookModel } from '../types';
13+
import { ExportDependencyChecker } from './exportDependencyChecker';
14+
import { ExportFileOpener } from './exportFileOpener';
715
import { ExportUtil } from './exportUtil';
816
import { ExportFormat, IExport, IExportManager, IExportManagerFilePicker } from './types';
917

@@ -16,10 +24,59 @@ export class ExportManager implements IExportManager {
1624
@inject(IFileSystem) private readonly fileSystem: IFileSystem,
1725
@inject(IExportManagerFilePicker) private readonly filePicker: IExportManagerFilePicker,
1826
@inject(ProgressReporter) private readonly progressReporter: ProgressReporter,
19-
@inject(ExportUtil) private readonly exportUtil: ExportUtil
27+
@inject(ExportUtil) private readonly exportUtil: ExportUtil,
28+
@inject(IApplicationShell) private readonly applicationShell: IApplicationShell,
29+
@inject(ExportFileOpener) private readonly exportFileOpener: ExportFileOpener,
30+
@inject(ExportDependencyChecker) private exportDepedencyChecker: ExportDependencyChecker
2031
) {}
2132

22-
public async export(
33+
public async export(format: ExportFormat, model: INotebookModel, defaultFileName?: string): Promise<undefined> {
34+
let target;
35+
try {
36+
await this.exportDepedencyChecker.checkDependencies(format);
37+
target = await this.getTargetFile(format, model, defaultFileName);
38+
if (!target) {
39+
return;
40+
}
41+
await this.performExport(format, model, target);
42+
} catch (e) {
43+
let msg = e;
44+
traceError('Export failed', e);
45+
sendTelemetryEvent(Telemetry.ExportNotebookAsFailed, undefined, { format: format });
46+
47+
if (format === ExportFormat.pdf) {
48+
msg = localize.DataScience.exportToPDFDependencyMessage();
49+
}
50+
51+
this.showExportFailed(msg);
52+
}
53+
}
54+
55+
private async performExport(format: ExportFormat, model: INotebookModel, target: Uri) {
56+
/* Need to make a temp directory here, instead of just a temp file. This is because
57+
we need to store the contents of the notebook in a file that is named the same
58+
as what we want the title of the exported file to be. To ensure this file path will be unique
59+
we store it in a temp directory. The name of the file matters because when
60+
exporting to certain formats the filename is used within the exported document as the title. */
61+
const tempDir = await this.exportUtil.generateTempDir();
62+
const source = await this.makeSourceFile(target, model, tempDir);
63+
64+
const reporter = this.progressReporter.createProgressIndicator(`Exporting to ${format}`, true);
65+
try {
66+
await this.exportToFormat(source, target, format, reporter.token);
67+
} finally {
68+
tempDir.dispose();
69+
reporter.dispose();
70+
}
71+
72+
if (reporter.token.isCancellationRequested) {
73+
sendTelemetryEvent(Telemetry.ExportNotebookAs, undefined, { format: format, cancelled: true });
74+
return;
75+
}
76+
await this.exportFileOpener.openFile(format, target);
77+
}
78+
79+
private async getTargetFile(
2380
format: ExportFormat,
2481
model: INotebookModel,
2582
defaultFileName?: string
@@ -28,56 +85,47 @@ export class ExportManager implements IExportManager {
2885

2986
if (format !== ExportFormat.python) {
3087
target = await this.filePicker.getExportFileLocation(format, model.file, defaultFileName);
31-
if (!target) {
32-
return;
33-
}
3488
} else {
3589
target = Uri.file((await this.fileSystem.createTemporaryFile('.py')).filePath);
3690
}
3791

38-
// Need to make a temp directory here, instead of just a temp file. This is because
39-
// we need to store the contents of the notebook in a file that is named the same
40-
// as what we want the title of the exported file to be. To ensure this file path will be unique
41-
// we store it in a temp directory. The name of the file matters because when
42-
// exporting to certain formats the filename is used within the exported document as the title.
92+
return target;
93+
}
94+
95+
private async makeSourceFile(target: Uri, model: INotebookModel, tempDir: TemporaryDirectory): Promise<Uri> {
96+
// Creates a temporary file with the same base name as the target file
4397
const fileName = path.basename(target.fsPath, path.extname(target.fsPath));
44-
const tempDir = await this.exportUtil.generateTempDir();
4598
const sourceFilePath = await this.exportUtil.makeFileInDirectory(model, `${fileName}.ipynb`, tempDir.path);
46-
const source = Uri.file(sourceFilePath);
99+
return Uri.file(sourceFilePath);
100+
}
47101

102+
private showExportFailed(msg: string) {
103+
// tslint:disable-next-line: messages-must-be-localized
104+
this.applicationShell.showErrorMessage(`${localize.DataScience.failedExportMessage()} ${msg}`).then();
105+
}
106+
107+
private async exportToFormat(source: Uri, target: Uri, format: ExportFormat, cancelToken: CancellationToken) {
48108
if (format === ExportFormat.pdf) {
49109
// When exporting to PDF we need to remove any SVG output. This is due to an error
50110
// with nbconvert and a dependency of its called InkScape.
51111
await this.exportUtil.removeSvgs(source);
52112
}
53113

54-
const reporter = this.progressReporter.createProgressIndicator(`Exporting to ${format}`, true);
55-
try {
56-
switch (format) {
57-
case ExportFormat.python:
58-
await this.exportToPython.export(source, target, reporter.token);
59-
break;
114+
switch (format) {
115+
case ExportFormat.python:
116+
await this.exportToPython.export(source, target, cancelToken);
117+
break;
60118

61-
case ExportFormat.pdf:
62-
await this.exportToPDF.export(source, target, reporter.token);
63-
break;
119+
case ExportFormat.pdf:
120+
await this.exportToPDF.export(source, target, cancelToken);
121+
break;
64122

65-
case ExportFormat.html:
66-
await this.exportToHTML.export(source, target, reporter.token);
67-
break;
123+
case ExportFormat.html:
124+
await this.exportToHTML.export(source, target, cancelToken);
125+
break;
68126

69-
default:
70-
break;
71-
}
72-
} finally {
73-
tempDir.dispose();
74-
reporter.dispose();
127+
default:
128+
break;
75129
}
76-
77-
if (reporter.token.isCancellationRequested) {
78-
return;
79-
}
80-
81-
return target;
82130
}
83131
}

‎src/client/datascience/export/types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ export enum ExportFormat {
99

1010
export const IExportManager = Symbol('IExportManager');
1111
export interface IExportManager {
12-
export(format: ExportFormat, model: INotebookModel, defaultFileName?: string): Promise<Uri | undefined>;
12+
export(format: ExportFormat, model: INotebookModel, defaultFileName?: string): Promise<undefined>;
1313
}
1414

1515
export const IExport = Symbol('IExport');

‎src/client/datascience/serviceRegistry.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ import { Decorator } from './editor-integration/decorator';
3636
import { HoverProvider } from './editor-integration/hoverProvider';
3737
import { DataScienceErrorHandler } from './errorHandler/errorHandler';
3838
import { ExportBase } from './export/exportBase';
39+
import { ExportDependencyChecker } from './export/exportDependencyChecker';
40+
import { ExportFileOpener } from './export/exportFileOpener';
3941
import { ExportManager } from './export/exportManager';
40-
import { ExportManagerDependencyChecker } from './export/exportManagerDependencyChecker';
41-
import { ExportManagerFileOpener } from './export/exportManagerFileOpener';
4242
import { ExportManagerFilePicker } from './export/exportManagerFilePicker';
4343
import { ExportToHTML } from './export/exportToHTML';
4444
import { ExportToPDF } from './export/exportToPDF';
@@ -293,9 +293,9 @@ export function registerTypes(serviceManager: IServiceManager) {
293293
serviceManager.addSingleton<IJupyterDebugService>(IJupyterDebugService, JupyterDebugService, Identifiers.RUN_BY_LINE_DEBUGSERVICE);
294294
serviceManager.add<IJupyterVariableDataProvider>(IJupyterVariableDataProvider, JupyterVariableDataProvider);
295295
serviceManager.addSingleton<IJupyterVariableDataProviderFactory>(IJupyterVariableDataProviderFactory, JupyterVariableDataProviderFactory);
296-
serviceManager.addSingleton<IExportManager>(ExportManager, ExportManager);
297-
serviceManager.addSingleton<IExportManager>(ExportManagerDependencyChecker, ExportManagerDependencyChecker);
298-
serviceManager.addSingleton<IExportManager>(IExportManager, ExportManagerFileOpener);
296+
serviceManager.addSingleton<IExportManager>(IExportManager, ExportManager);
297+
serviceManager.addSingleton<ExportDependencyChecker>(ExportDependencyChecker, ExportDependencyChecker);
298+
serviceManager.addSingleton<ExportFileOpener>(ExportFileOpener, ExportFileOpener);
299299
serviceManager.addSingleton<IExport>(IExport, ExportToPDF, ExportFormat.pdf);
300300
serviceManager.addSingleton<IExport>(IExport, ExportToHTML, ExportFormat.html);
301301
serviceManager.addSingleton<IExport>(IExport, ExportToPython, ExportFormat.python);

‎src/test/datascience/dataScienceIocContainer.ts

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -201,9 +201,9 @@ import { CodeWatcher } from '../../client/datascience/editor-integration/codewat
201201
import { HoverProvider } from '../../client/datascience/editor-integration/hoverProvider';
202202
import { DataScienceErrorHandler } from '../../client/datascience/errorHandler/errorHandler';
203203
import { ExportBase } from '../../client/datascience/export/exportBase';
204+
import { ExportDependencyChecker } from '../../client/datascience/export/exportDependencyChecker';
205+
import { ExportFileOpener } from '../../client/datascience/export/exportFileOpener';
204206
import { ExportManager } from '../../client/datascience/export/exportManager';
205-
import { ExportManagerDependencyChecker } from '../../client/datascience/export/exportManagerDependencyChecker';
206-
import { ExportManagerFileOpener } from '../../client/datascience/export/exportManagerFileOpener';
207207
import { ExportManagerFilePicker } from '../../client/datascience/export/exportManagerFilePicker';
208208
import { ExportToHTML } from '../../client/datascience/export/exportToHTML';
209209
import { ExportToPDF } from '../../client/datascience/export/exportToPDF';
@@ -567,20 +567,18 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
567567
instance(this.webPanelProvider)
568568
);
569569
}
570-
this.serviceManager.addSingleton<IExportManager>(ExportManager, ExportManager);
571-
this.serviceManager.addSingleton<IExportManager>(
572-
ExportManagerDependencyChecker,
573-
ExportManagerDependencyChecker
574-
);
575-
this.serviceManager.addSingleton<ExportUtil>(ExportUtil, ExportUtil);
576-
this.serviceManager.addSingleton<INotebookModelFactory>(INotebookModelFactory, NotebookModelFactory);
577-
this.serviceManager.addSingleton<IExportManager>(IExportManager, ExportManagerFileOpener);
570+
this.serviceManager.addSingleton<IExportManager>(IExportManager, ExportManager);
571+
this.serviceManager.addSingleton<ExportDependencyChecker>(ExportDependencyChecker, ExportDependencyChecker);
572+
this.serviceManager.addSingleton<ExportFileOpener>(ExportFileOpener, ExportFileOpener);
578573
this.serviceManager.addSingleton<IExport>(IExport, ExportToPDF, ExportFormat.pdf);
579574
this.serviceManager.addSingleton<IExport>(IExport, ExportToHTML, ExportFormat.html);
580575
this.serviceManager.addSingleton<IExport>(IExport, ExportToPython, ExportFormat.python);
581576
this.serviceManager.addSingleton<IExport>(IExport, ExportBase, 'Export Base');
577+
this.serviceManager.addSingleton<ExportUtil>(ExportUtil, ExportUtil);
582578
this.serviceManager.addSingleton<ExportCommands>(ExportCommands, ExportCommands);
583579
this.serviceManager.addSingleton<IExportManagerFilePicker>(IExportManagerFilePicker, ExportManagerFilePicker);
580+
581+
this.serviceManager.addSingleton<INotebookModelFactory>(INotebookModelFactory, NotebookModelFactory);
584582
this.serviceManager.addSingleton<IMountedWebViewFactory>(IMountedWebViewFactory, MountedWebViewFactory);
585583
this.registerFileSystemTypes();
586584
this.serviceManager.rebindInstance<IFileSystem>(IFileSystem, new MockFileSystem());

0 commit comments

Comments
 (0)