Skip to content

Handling Save as of notebooks when using new VSC Notebook API #11876

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

Merged
merged 13 commits into from
May 18, 2020

Conversation

DonJayamanne
Copy link

@DonJayamanne DonJayamanne commented May 18, 2020

For #10496

  • Partial implementation of Save As.
  • There's more to this, but wanted to get in what I had.

Final implementation details outlined here:

* master:
  Sync VSCode output Cell Output & add INotebookEditor/INotebookEditorProvider (#11849)
  Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang (#11816)
  Debug and variable test failures on nightly (#11848)
  Add cell execution to the new VSC Notebooks (#11846)
  Prelimnary support to load ipyn files in native Notebook editor (#11841)
  Fix memleaks during tests (#11835)
  turn on raw kernel for insiders (#11820)
  Some fixes for nightly (#11802)
  correctly hook up session status in raw kernel (#11801)
  Remove waitForUpdate from tests to make them more deterministic (#11799)
  Fix run by line to work for restart/change kernel (#11795)
  Clarify that using a custom jedi also means a custom parso (#11708)
  Fix typo lanaguage -> language (#11720)
  Remove tpn (#11765)
  on raw kernel launch wait for ready, and have raw kernel switch respect timeout and retry (#11768)
  Allow azure files and only nest scrolling in notebooks (#11771)
@DonJayamanne DonJayamanne added the no-changelog No news entry required label May 18, 2020
@DonJayamanne DonJayamanne marked this pull request as ready for review May 18, 2020 16:36
@DonJayamanne DonJayamanne changed the title WIP Handling Save as of notebooks when using new VSC Notebook API May 18, 2020
// tslint:disable-next-line: no-suspicious-comment
// TODO: This will not work, if we close an untitled document.
// See if we have any untitled storage already
const untitledStorage = Array.from(this.models.values()).filter(isUntitled);
Copy link

Choose a reason for hiding this comment

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

Why not use a counter? Aren't you mimicing the existing bug with this?

Copy link
Author

Choose a reason for hiding this comment

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

u mimicing the existing bug with this?

Yes, didn't want to fix it here. Just want to leave the bug and fix it correctly with tests. Doing that means more work at this stage.
This is being tracked, hence leaving this.

Copy link

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

🕐

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.

Approved with a couple of small comments.

@DonJayamanne DonJayamanne requested a review from rchiodo May 18, 2020 17:40
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.8% 1.8% Duplication

Copy link

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

:shipit:

@DonJayamanne DonJayamanne merged commit 1fcb309 into microsoft:master May 18, 2020
@DonJayamanne DonJayamanne deleted the more branch May 18, 2020 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants