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

removed Notebook creation from extension since it is moving to built-in #9311

Merged
merged 5 commits into from
Mar 15, 2022

Conversation

amunger
Copy link
Contributor

@amunger amunger commented Mar 9, 2022

Fixes #9250

This should be merged in coordination with microsoft/vscode#144789 to limit the amount of time that the command is unavailable/duplicated.

The create Notebook flow is still used pretty widely in the test framework, so this change does not completely remove it.

This will remove the functionality of registered 3rd party notebooks appearing in the "Create Jupyter Notebook" quick pick menu since that command will simply create a Python Notebook.

  • 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).

@DonJayamanne
Copy link
Contributor

"Create Jupyter Notebook" quick pick menu since that command will simply create a Python Notebook.

Thats not true, if you open a julia notebook and use that, then the new notebook created using that command defaults to Julia.
Similarly if you are an R user, the new notebook defaults to R & not python.

@amunger
Copy link
Contributor Author

amunger commented Mar 14, 2022

the new notebook defaults to R & not python

That happens through controller selection for the created notebook, not the notebook creation itself. This change should not affect that behavior, although I'm having a hard time testing that in OSS.

@DonJayamanne
Copy link
Contributor

Thats not true, if you open a julia notebook and use that, then the new notebo

Yeah looking at the old code, it never worked, looks like the code was lost at some point.
Surprised no one is complaining, I guess it works because untitled notebooks automatically have their kernel change to what they used last, and that results in cell languages getting updated.

@amunger amunger force-pushed the dev/aamunger/CreateNotebook branch from 05aba4f to 62010e4 Compare March 15, 2022 16:18
@amunger amunger marked this pull request as ready for review March 15, 2022 17:08
@amunger amunger requested a review from a team as a code owner March 15, 2022 17:08
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2022

Codecov Report

Merging #9311 (48a4e41) into main (f03d91f) will increase coverage by 0%.
The diff coverage is n/a.

❗ Current head 48a4e41 differs from pull request most recent head 4d497be. Consider uploading reports for the commit 4d497be to get more accurate results

@@          Coverage Diff          @@
##            main   #9311   +/-   ##
=====================================
  Coverage     74%     74%           
=====================================
  Files        219     219           
  Lines      10919   10919           
  Branches    1512    1512           
=====================================
+ Hits        8153    8183   +30     
+ Misses      2176    2138   -38     
- Partials     590     598    +8     
Impacted Files Coverage Δ
src/client/datascience/dataScienceSurveyBanner.ts 69% <0%> (-7%) ⬇️
src/client/common/process/pythonDaemonPool.ts 76% <0%> (+1%) ⬆️
...ent/datascience/progress/kernelProgressReporter.ts 80% <0%> (+2%) ⬆️
src/client/common/process/proc.ts 78% <0%> (+2%) ⬆️
.../client/datascience/plotting/plotViewerProvider.ts 41% <0%> (+2%) ⬆️
src/client/common/featureDeprecationManager.ts 37% <0%> (+3%) ⬆️
src/client/extension.ts 61% <0%> (+3%) ⬆️
src/client/datascience/multiplexingDebugService.ts 55% <0%> (+3%) ⬆️
src/client/datascience/datascience.ts 95% <0%> (+4%) ⬆️
...ient/datascience/data-viewing/dataViewerFactory.ts 87% <0%> (+4%) ⬆️
... and 3 more

Copy link
Contributor

@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.

We must change the engine in package.json (to 1.65-insider) to ensure this change goes out ONLY with the corresponding 1.65 version of VS Code.

@amunger amunger merged commit 502897a into main Mar 15, 2022
@amunger amunger deleted the dev/aamunger/CreateNotebook branch March 15, 2022 23:25
amunger added a commit that referenced this pull request Mar 16, 2022
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.

Move "New Jupyter File" (ipynb) command and implementation to VS Code core
5 participants