Skip to content

Fix a number of issues related to opening new notebooks #8270

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 7 commits into from
Oct 29, 2019

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Oct 29, 2019

For #8132, #7980, #8263

Fixes for

  • Creating multiple notebooks.
  • Creating a new notebook and having it have some other data in it
  • Creating a notebook, having the extension upgrade, and losing all your changes.

Fix opening unsaved notebooks after extension update
Fix creating new notebooks to be empty
Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

I think there's an issue

@codecov-io
Copy link

codecov-io commented Oct 29, 2019

Codecov Report

Merging #8270 into master will increase coverage by 0.03%.
The diff coverage is 52.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8270      +/-   ##
==========================================
+ Coverage   59.29%   59.32%   +0.03%     
==========================================
  Files         504      504              
  Lines       23071    23087      +16     
  Branches     3732     3736       +4     
==========================================
+ Hits        13680    13697      +17     
+ Misses       8502     8498       -4     
- Partials      889      892       +3
Impacted Files Coverage Δ
...ascience/interactive-ipynb/nativeEditorProvider.ts 54.54% <30%> (+5.42%) ⬆️
...ient/datascience/interactive-ipynb/nativeEditor.ts 58.76% <85.71%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d47bcc...e1d7906. Read the comment docs.

@rchiodo rchiodo closed this Oct 29, 2019
@rchiodo rchiodo reopened this Oct 29, 2019

test('Opening file with local storage but no global will still open with old contents', async () => {
// This test is really for making sure when a user upgrades to a new extension, we still have their old storage
const file = Uri.parse('file://foo.ipynb');
Copy link
Member

Choose a reason for hiding this comment

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

Take a peek at this in the debugger quick. I think the specification is incorrect here (though the test might pass). 'file://foo.ipynb' doesn't set the fsPath and puts the filename in the authority. 'file:///foo.ipynb' works.

Copy link
Author

Choose a reason for hiding this comment

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

Yes you're right. The file name comes out as /. I'll change it and see if the tests still pass

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, when you do that could you take a look at the other places that we do that in this file? Just so we don't get bit by this later? I should have already done that with my checkin when I noticed the issue, but I forgot to add them in.


In reply to: 340233792 [](ancestors = 340233792)

@rchiodo rchiodo merged commit cd56738 into master Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 5, 2019
@rchiodo rchiodo deleted the rchiodo/blank_open branch November 11, 2019 17:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants