Skip to content

Allow for changing the jupyter command line that we use#10062

Merged
rchiodo merged 5 commits into
masterfrom
rchiodo/jupyter_command_line
Feb 12, 2020
Merged

Allow for changing the jupyter command line that we use#10062
rchiodo merged 5 commits into
masterfrom
rchiodo/jupyter_command_line

Conversation

@rchiodo
Copy link
Copy Markdown

@rchiodo rchiodo commented Feb 11, 2020

For #8698

Added a new command to set the command line we use for starting jupyter.

@rchiodo rchiodo self-assigned this Feb 11, 2020
@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 12, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@7a5fbd2). Click here to learn what that means.
The diff coverage is 66.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #10062   +/-   ##
=========================================
  Coverage          ?   61.31%           
=========================================
  Files             ?      567           
  Lines             ?    30641           
  Branches          ?     4391           
=========================================
  Hits              ?    18789           
  Misses            ?    10872           
  Partials          ?      980
Impacted Files Coverage Δ
src/client/common/application/types.ts 100% <ø> (ø)
...ger/extension/configuration/providers/pidAttach.ts 100% <ø> (ø)
src/client/debugger/types.ts 100% <ø> (ø)
...c/client/debugger/extension/configuration/types.ts 100% <ø> (ø)
...ommon/terminal/shellDetectors/baseShellDetector.ts 100% <ø> (ø)
src/client/common/terminal/types.ts 100% <ø> (ø)
src/client/activation/types.ts 100% <ø> (ø)
...ience/interactive-common/interactiveWindowTypes.ts 100% <ø> (ø)
src/client/debugger/extension/adapter/logging.ts 97.14% <ø> (ø)
src/client/common/process/types.ts 100% <ø> (ø)
... and 241 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 7a5fbd2...5206c3c. Read the comment docs.

DonJayamanne
DonJayamanne requested changes Feb 12, 2020
View reviewed changes
Copy link
Copy Markdown

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

With more configuration options, we also end up with more points of failure.
I'm concerned about Jupyter not starting because of this, user accidentally entering incorrect arguments.
Should we add users to check their arguments figure out why Jupyter isn't starting?
Or look for error messages that indicates some faulty argument in the cli...

Or is there a way to validate the arguments as being valid Jupyter arguments..

Comment thread src/client/datascience/jupyter/commandLineSelector.ts Outdated
Comment thread src/client/datascience/jupyter/commandLineSelector.ts
Comment thread src/test/datascience/jupyterVariables.unit.test.ts Outdated
@rchiodo
Copy link
Copy Markdown
Author

rchiodo commented Feb 12, 2020

It's buyer beware. You enter the wrong arguments and jupyter might not start. Same as turning off the default config we generate.

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

@rchiodo
Copy link
Copy Markdown
Author

rchiodo commented Feb 12, 2020

I could make the picker and the setting specified using the default as 'recommended'

In reply to: 585285382 [](ancestors = 585285382,357132930)

@rchiodo
Copy link
Copy Markdown
Author

rchiodo commented Feb 12, 2020

And this option is the ONLY option to get some people's system working.

In reply to: 585285776 [](ancestors = 585285776,585285382,357132930)

IanMatthewHuff
IanMatthewHuff reviewed Feb 12, 2020
View reviewed changes
Comment thread src/client/datascience/jupyter/notebookStarter.ts
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Feb 12, 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

@rchiodo
Copy link
Copy Markdown
Author

rchiodo commented Feb 12, 2020

@DonJayamanne did you have more changes? I changed it to an array.

@rchiodo rchiodo merged commit 97b1c7a into master Feb 12, 2020
@lock lock Bot locked as resolved and limited conversation to collaborators Feb 21, 2020
@rchiodo rchiodo deleted the rchiodo/jupyter_command_line branch February 28, 2020 17:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@IanMatthewHuff IanMatthewHuff IanMatthewHuff approved these changes

@DonJayamanne DonJayamanne DonJayamanne approved these changes

@DavidKutu DavidKutu Awaiting requested review from DavidKutu

Assignees

@rchiodo rchiodo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rchiodo @codecov-io @IanMatthewHuff @DonJayamanne