Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix export to use the source file for the directory it wants to open #4072

Merged
merged 5 commits into from
Dec 2, 2020

Conversation

rchiodo
Copy link
Contributor

@rchiodo rchiodo commented Dec 2, 2020

For #3991

Rework how export is done to not have to use INotebookModel anymore. This allows the decoupling of the model from the file it has (necessary for the interactive window to export correctly).

Then change all exporting paths to go through the same 'IExportDialog' class which will use the source file to pick the directory to export to.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).

@rchiodo rchiodo requested a review from a team as a code owner December 2, 2020 01:07
export const PythonExtensions = { Python: ['py'] };

@injectable()
export class ExportDialog implements IExportDialog {
Copy link
Contributor Author

@rchiodo rchiodo Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file used to be called ExportManagerFilePicker. I thought this name made more sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bleh, github didn't pick up the rename though? Can't easily tell what changed...

} finally {
this.stopProgress();
// Pull out the metadata from our active notebook
const metadata: nbformat.INotebookMetadata = { orig_nbformat: 3 };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't working before. Now the exported notebook will have the actual kernel inside of it.

@@ -96,5 +96,5 @@ export async function computeWorkingDirectory(resource: Resource, workspace: IWo
}

// Otherwise a file without an extension or directory doesn't exist. Just use the workspace root
return workspace.getWorkspaceFolder(resource)?.uri.fsPath || process.cwd();
return workspace.getWorkspaceFolder(resource)?.uri.fsPath || workspace.rootPath || process.cwd();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems getWorkspaceFolder only works if you're in a workspace.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - Under the hood workspace.rootPath also uses workspace.worksapceFolders, hence if you don't have a workspace folder opened, then workspace.root will still return undefined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change required.

exportMethod?: ExportFormat,
defaultFileName?: string,
interpreter?: PythonEnvironment
) {
if (!model) {
// if no model was passed then this was called from the command palette,
if (!contents || !source) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check calling this Export command from the command palette with all our types of editor? I thought when I was looking at this recently there was a case where a Model was getting passed in. I think that it might have been Old editor active editor = Command is called with all undefined. CustomEditor active editor = Command is called with an INotebookModel. Not 100% sure I'm recalling correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this should work (I'll double check). This happens when you try to export a notebook from the command palette (instead of from our toolbar).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thing I double checked. Found another issue. Interactive window would show the export commands if a notebook had ever been opened. Since the active editor here is from the notebook, the export command would essentially do nothing.

The bug was in our tracking of active editor. We weren't firing the change of the active editor on close.

@DonJayamanne
Copy link
Contributor

Like the new changes.

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good with just that one thing to check.

@rchiodo rchiodo merged commit ff1e6cd into main Dec 2, 2020
@rchiodo rchiodo deleted the rchiodo/export_path branch December 2, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants