Skip to content

Streamline DS Dependency installs #11717

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.

Sign up for GitHub

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

Jump to bottom
Merged
greazer merged 6 commits into from
May 11, 2020
Merged

Streamline DS Dependency installs #11717

greazer merged 6 commits into from
May 11, 2020

Conversation

@greazer
Copy link
Member

@greazer greazer commented May 9, 2020 •
edited
Loading

#11650 Streamline how we prompt to install ipykernel.

There are two options represented in this branch:

  1. Ask for Conda/Pip in the when we prompt that ipykernel needs to be installed.
  2. Just allow the user to decide whether ipykernel should be installed and automatically choose conda or pip.

The latest version of this PR represents the second option, which I think is the better choice. The first option is represented in the first commit of this branch.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • [x] 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.
  • [x] Test plan is updated as appropriate.
  • [x] package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • [x] The wiki is updated with any design decisions/details.

@greazer greazer added the no-changelog No news entry required label May 9, 2020
@codecov-io
Copy link

codecov-io commented May 9, 2020 •
edited
Loading

Codecov Report

Merging #11717 into master will decrease coverage by 0.04%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11717      +/-   ##
==========================================
- Coverage   60.71%   60.67%   -0.05%     
==========================================
  Files         629      629              
  Lines       33999    34024      +25     
  Branches     4793     4799       +6     
==========================================
  Hits        20643    20643              
- Misses      12350    12372      +22     
- Partials     1006     1009       +3
Impacted Files Coverage Δ
...science/jupyter/kernels/kernelDependencyService.ts 89.28% <0.00%> (ø)
src/client/common/installer/productInstaller.ts 86.16% <26.31%> (-6.67%) ⬇️
src/client/common/utils/localize.ts 95.54% <100.00%> (+<0.01%) ⬆️
src/datascience-ui/react-common/arePathsSame.ts 75.00% <0.00%> (-12.50%) ⬇️
src/client/common/utils/platform.ts 64.70% <0.00%> (-11.77%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0.00%> (-2.23%) ⬇️
src/client/datascience/debugLocationTracker.ts 76.56% <0.00%> (-1.57%) ⬇️
src/client/common/process/proc.ts 14.49% <0.00%> (-0.73%) ⬇️
src/client/datascience/jupyter/jupyterImporter.ts 15.90% <0.00%> (-0.57%) ⬇️
... and 5 more

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 84f9c7a...7659bc6. Read the comment docs.

rchiodo
rchiodo approved these changes May 11, 2020
View reviewed changes
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 requested changes May 11, 2020
View reviewed changes
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.

Need to display error to user if unable to install, else all is good.

IanMatthewHuff
IanMatthewHuff reviewed May 11, 2020
View reviewed changes
DonJayamanne
DonJayamanne requested changes May 11, 2020
View reviewed changes
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.

@IanMatthewHuff Thanks.

IanMatthewHuff
IanMatthewHuff approved these changes May 11, 2020
View reviewed changes
Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approved as long as we toss an error on the URI case being passed in.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 11, 2020

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
0.0% 0.0% Duplication

@greazer greazer merged commit 8c4d432 into master May 11, 2020
@greazer greazer deleted the greazer/StreamlineDependencyInstall branch May 11, 2020 20:04
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DonJayamanne DonJayamanne DonJayamanne requested changes

@IanMatthewHuff IanMatthewHuff IanMatthewHuff approved these changes

@rchiodo rchiodo rchiodo approved these changes

+1 more reviewer

@DavidKutu DavidKutu DavidKutu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

no-changelog No news entry required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@greazer @codecov-io @IanMatthewHuff @DonJayamanne @rchiodo @DavidKutu