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

Modify SelectJupyterURI to allow URI parameter #8918

Merged
merged 7 commits into from
Feb 4, 2022

Conversation

C-SELLERS
Copy link
Contributor

@C-SELLERS C-SELLERS commented Feb 3, 2022

For #8902

Made the change to allow a bool and URI to be passed to command jupyter.selectjupyteruri. If source is a Uri it sets the serverSelector to the remote uri provided by the user.

I added the bool mostly because it seemed like that was what it was originally setup to do. But the arg was unused because this.serverSelector.selectJupyterURI(true, source), had true hardcoded. It might not be necessary but it does allow someone to call jupyter.selectjupyteruri and have their selector not include local options.

I also added a test for this case in the commands unit test for the serverSelector.

  • 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. : N/A
  • package-lock.json has been regenerated by running npm install (if dependencies have changed). N/A

@C-SELLERS C-SELLERS requested a review from a team as a code owner February 3, 2022 21:44
@ghost
Copy link

ghost commented Feb 3, 2022

CLA assistant check
All CLA requirements met.

@C-SELLERS C-SELLERS requested a review from rchiodo February 3, 2022 22:26
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022

Codecov Report

Merging #8918 (f8d20c4) into main (e9eb7e1) will decrease coverage by 1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##            main   #8918    +/-   ##
======================================
- Coverage     71%     70%    -2%     
======================================
  Files        382     382            
  Lines      24750   24391   -359     
  Branches    3826    3850    +24     
======================================
- Hits       17718   17204   -514     
- Misses      5503    5663   +160     
+ Partials    1529    1524     -5     
Impacted Files Coverage Δ
src/client/datascience/commands/serverSelector.ts 90% <100%> (+3%) ⬆️
src/client/debugger/jupyter/debugControllers.ts 20% <0%> (-43%) ⬇️
...ience/variablesView/variableViewMessageListener.ts 77% <0%> (-23%) ⬇️
src/client/debugger/jupyter/kernelDebugAdapter.ts 57% <0%> (-20%) ⬇️
src/client/debugger/jupyter/helper.ts 41% <0%> (-20%) ⬇️
...ent/common/application/webviewViews/webviewView.ts 67% <0%> (-11%) ⬇️
...rc/client/datascience/jupyter/debuggerVariables.ts 58% <0%> (-8%) ⬇️
...ient/datascience/jupyter/kernels/kernelProvider.ts 85% <0%> (-5%) ⬇️
.../client/datascience/debugLocationTrackerFactory.ts 92% <0%> (-4%) ⬇️
...atascience/interactive-window/interactiveWindow.ts 66% <0%> (-3%) ⬇️
... and 26 more

@rchiodo
Copy link
Contributor

rchiodo commented Feb 3, 2022

You should fix the linter problems. The other tests failing are a known issue and not caused by your change. You can ignore those.

@rchiodo rchiodo merged commit f9329d6 into microsoft:main Feb 4, 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.

3 participants