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

Remove INotebook interface #9878

Merged
merged 8 commits into from
May 3, 2022
Merged

Remove INotebook interface #9878

merged 8 commits into from
May 3, 2022

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented May 3, 2022

Part of #8933

The INotebookProvider will remain for now, this is just step 1 of refactoring to remove/refactor some of the old code related to live share.
Note: INotebook had two properties & one of the was IJupyterSesion & the other connection, & connection was never used, hence INotebook interface was completely unnecessary, we could have just one property session or remote it all together - went with the latter.

I don't want to rename INotebookProvider to ISessionProvider as we already have a interface named ISessionManager & both have create functions.

The Notebook provider does a few things that we an split, e.g. starting of Jupyter for local non-raw cases, or it can be done by other means, either way, that can be done seprately (and a few more changes to simplify things).

TLDR - this only focuses on removing INotebook

@DonJayamanne DonJayamanne requested a review from a team as a code owner May 3, 2022 19:09
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2022

Codecov Report

Merging #9878 (9c8fac9) into main (131f6c3) will increase coverage by 0%.
The diff coverage is n/a.

❗ Current head 9c8fac9 differs from pull request most recent head f53447c. Consider uploading reports for the commit f53447c to get more accurate results

@@         Coverage Diff          @@
##           main   #9878   +/-   ##
====================================
  Coverage    63%     63%           
====================================
  Files       203     203           
  Lines      9826    9826           
  Branches   1564    1564           
====================================
+ Hits       6243    6249    +6     
+ Misses     3079    3071    -8     
- Partials    504     506    +2     
Impacted Files Coverage Δ
...rc/platform/common/dataScienceSurveyBanner.node.ts 70% <0%> (+5%) ⬆️

@DonJayamanne DonJayamanne merged commit 9119d24 into main May 3, 2022
@DonJayamanne DonJayamanne deleted the removeINotebook branch May 3, 2022 20:17
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.

3 participants