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

Stop asking users to install ipykernel on autostart #5440

Merged
merged 7 commits into from
Apr 8, 2021

Conversation

DavidKutu
Copy link

only do it when a cell is ran.

For #5368

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

@DavidKutu DavidKutu requested a review from a team as a code owner April 7, 2021 21:06
Copy link
Contributor

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

I think createNotebookIfProviderConnectionExists should be changed to add a flag and it should only be set to true in the ctor call.

This also means in the same function the call to this.notebookProvider.connect should also pass the disable UI flag.

- pass true in constructors
@DavidKutu DavidKutu requested a review from rchiodo April 8, 2021 16:34
@DavidKutu DavidKutu requested a review from rchiodo April 8, 2021 17:06
@codecov-io
Copy link

codecov-io commented Apr 8, 2021

Codecov Report

Merging #5440 (2ae8311) into main (54bc65d) will decrease coverage by 0%.
The diff coverage is 100%.

❗ Current head 2ae8311 differs from pull request most recent head 961d913. Consider uploading reports for the commit 961d913 to get more accurate results

@@          Coverage Diff          @@
##            main   #5440   +/-   ##
=====================================
- Coverage     73%     73%   -1%     
=====================================
  Files        401     401           
  Lines      26584   26585    +1     
  Branches    3880    3882    +2     
=====================================
- Hits       19510   19509    -1     
- Misses      5459    5460    +1     
- Partials    1615    1616    +1     
Impacted Files Coverage Δ
src/client/datascience/types.ts 100% <ø> (ø)
.../datascience/interactive-common/interactiveBase.ts 70% <100%> (ø)
...datascience/interactive-common/notebookProvider.ts 81% <100%> (ø)
...ient/datascience/interactive-ipynb/nativeEditor.ts 70% <100%> (ø)
...ient/datascience/raw-kernel/rawNotebookProvider.ts 90% <100%> (ø)
src/client/common/errors/errorUtils.ts 71% <0%> (-4%) ⬇️
...datascience/editor-integration/codelensprovider.ts 85% <0%> (-2%) ⬇️
...e/interactive-ipynb/nativeEditorCommandListener.ts 44% <0%> (-2%) ⬇️
src/client/datascience/baseJupyterSession.ts 75% <0%> (-1%) ⬇️
...ient/datascience/editor-integration/codewatcher.ts 69% <0%> (-1%) ⬇️
... and 4 more

@DavidKutu DavidKutu requested a review from rchiodo April 8, 2021 18:17
@DavidKutu DavidKutu closed this Apr 8, 2021
@DavidKutu DavidKutu reopened this Apr 8, 2021
@DavidKutu DavidKutu closed this Apr 8, 2021
@DavidKutu DavidKutu reopened this Apr 8, 2021
@DavidKutu DavidKutu merged commit 3cdfcaa into main Apr 8, 2021
@DavidKutu DavidKutu deleted the david/ipykernelOnAutoStart branch April 8, 2021 19:21
DavidKutu pushed a commit that referenced this pull request Apr 9, 2021
* Stop asking users to install ipykernel on autostart,
only do it when a cell is ran.

* - make disableUI false for default
- pass true in constructors

* - pass the option to disableUI around more

* lint

* oops

* one more spot

* remove some comments
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