Refactored exporting notebooks to python script #12232
Conversation
techwithtim
changed the title
| tempFile.dispose(); | ||
| } | ||
| status.dispose(); | ||
| private async exportAs(): Promise<void> { |
There was a problem hiding this comment.
I believe you need this equivalent function in the interactive window.
There was a problem hiding this comment.
I've left the current export method in interactive window for now. Can't remember exactly why but I think interactive window behaves differently so I didn't change it. Here's what the function in interactive window looks like:
@captureTelemetry(Telemetry.ExportNotebook, undefined, false)
// tslint:disable-next-line: no-any no-empty
private async export(cells: ICell[]) {
// Should be an array of cells
if (cells && this.applicationShell) {
// Indicate busy
this.startProgress();
try {
const filtersKey = localize.DataScience.exportDialogFilter();
const filtersObject: Record<string, string[]> = {};
filtersObject[filtersKey] = ['ipynb'];
// Bring up the open file dialog box
const uri = await this.applicationShell.showSaveDialog({
saveLabel: localize.DataScience.exportDialogTitle(),
filters: filtersObject
});
if (uri) {
await this.jupyterExporter.exportToFile(cells, uri.fsPath);
}
} finally {
this.stopProgress();
}
}
}There was a problem hiding this comment.
Seems like it should work the same though. I think people would want export to pdf/html for the interactive window too. Oh, 1 difference. Interactive exports to ipynb. Native exports to .py file.
There was a problem hiding this comment.
Seems like we should wire it up before we're all done though.
src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts
Outdated
Show resolved
Hide resolved
| const activeTextEditorChange = createDeferred(); | ||
| const docManager = ioc.get<IDocumentManager>(IDocumentManager) as MockDocumentManager; | ||
| docManager.onDidChangeActiveTextEditor(() => activeTextEditorChange.resolve()); | ||
| const commandFired = createDeferred(); |
There was a problem hiding this comment.
This seems like the test isn't actually testing export anymore. It looks like it's just testing that the command fires. This should still work without having to rewire the command manager. Perhaps the arguments aren't getting passed around?
There was a problem hiding this comment.
I could not pass these test without doing this. Don and I worked on it, I'll wait for his feedback here @DonJayamanne
There was a problem hiding this comment.
I can help debug if you like. This test should be pretty straightforward to debug what's happening.
There was a problem hiding this comment.
We have a separate test for exporting a ipynb to python file and opening it as well.
That's an end to end test, just not testing from here.
This way, we don't need to write such a test again for native notebook or interactive window.
I.e. here we validate whether command is called.
In another place we test exporting.
Please see here (super simple test) https://github.com/microsoft/vscode-python/pull/12232/files/12ec2072888b3f75aa8a2f24d39677cfe3020ead#diff-0135d4b7cc537d39be915ab50526a81a
There was a problem hiding this comment.
That commit doesn't work. Where's the other end to end export test?
In reply to: 439532742 [](ancestors = 439532742)
There was a problem hiding this comment.
There's an export test for the interactive window. (notebook.functional.tests.ts), but I don't see one for convert to python.
In reply to: 440325263 [](ancestors = 440325263,439532742)
There was a problem hiding this comment.
src/test/datascience/export/exportManagerFileOpener.unit.test.ts
src/test/datascience/export/exportToPython.test.ts
Did you see those?
There was a problem hiding this comment.
Nope didn't notice those. Theoretically there could be a disconnect between the command being handled and the actual export happening, but the sum of those tests and this one do test all of the code. However you could eliminate those other tests if you got this one working end to end. And then we'd have a true end to end on export. Does that not sound better?
In reply to: 440343334 [](ancestors = 440343334)
There was a problem hiding this comment.
Does sound better. Can that wait for another PR? I'm planning on adding more tests and other enhancements in future PR's
There was a problem hiding this comment.
DonJayamanne
changed the title
|
|
||
| export const IExportManagerFilePicker = Symbol('IExportManagerFilePicker'); | ||
| export interface IExportManagerFilePicker { | ||
| getExportFileLocation(format: ExportFormat): Promise<Uri | undefined>; |
There was a problem hiding this comment.
| getExportFileLocation(format: ExportFormat): Promise<Uri | undefined>; | |
| getExportFileLocation(model:INotebookModel, format: ExportFormat): Promise<Uri | undefined>; |
Personally I'd prefer to pass in the file that's being exported.
This way when we display the save dialog we can generate a file based on the existing name.
E.g. if the nb is pytorch.ipynb, i'd expect the save diaglog to default the file name to pytorch.pdf (if format is pdf).
Oh yes, generally I'd see a custom title in the save diaglog such as Export pytorch.ipyn to PDF or the like..
| const activeTextEditorChange = createDeferred(); | ||
| const docManager = ioc.get<IDocumentManager>(IDocumentManager) as MockDocumentManager; | ||
| docManager.onDidChangeActiveTextEditor(() => activeTextEditorChange.resolve()); | ||
| const commandFired = createDeferred(); |
There was a problem hiding this comment.
We have a separate test for exporting a ipynb to python file and opening it as well.
That's an end to end test, just not testing from here.
This way, we don't need to write such a test again for native notebook or interactive window.
I.e. here we validate whether command is called.
In another place we test exporting.
Please see here (super simple test) https://github.com/microsoft/vscode-python/pull/12232/files/12ec2072888b3f75aa8a2f24d39677cfe3020ead#diff-0135d4b7cc537d39be915ab50526a81a
|
Kudos, SonarCloud Quality Gate passed!
|
techwithtim
changed the title
| ignoreFocusOut: false, | ||
| matchOnDescription: true, | ||
| matchOnDetail: true, | ||
| placeHolder: 'Export As...' |
| return; | ||
| } | ||
|
|
||
| const notebookFileName = stripFileExtension(path.basename(source.fsPath)); |
There was a problem hiding this comment.
use path.basename(soruce.fsPath, path.extname(source.fsapat))
Pass the second argument into path.basename
| this.defaultExportSaveLocation | ||
| ); | ||
|
|
||
| return Uri.file(filePath); |
There was a problem hiding this comment.
What will happen if a user exports a python file for the first time?
Will this code exist?
|
|
||
| private updateFileSaveLocation(value: Uri) { | ||
| const location = path.dirname(value.fsPath); | ||
| this.workspaceStorage.update(ExportNotebookSettings.lastSaveLocation, location); |
There was a problem hiding this comment.
I believe this is async code, you need an await here.
For #
This PR added the necessary infrastructure for exporting python files to different formats (specifically PDF and HTML). It also includes the implementation of exporting to python script which works as before, it has just been rewritten.
NOTE: Telemetry and more tests will be added in a future PR.
@techwithtim will continue to work on this and add implementations for export to PDF and HTML in the coming days.
Has a news entry file (remember to thank yourself!).Has sufficient logging.Has telemetry for enhancements.Test plan is updated as appropriate.package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).The wiki is updated with any design decisions/details.