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

Ensure launchingFile exists when setting working directory #6577

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

joyceerhl
Copy link
Contributor

@joyceerhl joyceerhl commented Jul 7, 2021

For #6549

With fix, interactive window now reflects working directory of the launching file:

image

@DonJayamanne I saw that this line actually used to be launchingFile && await this.fs.localFileExists(launchingFile) and you changed it to launchingFile && (await this.fs.localDirectoryExists(path.dirname(launchingFile)) in https://github.com/microsoft/vscode-jupyter/pull/5911/files#diff-a0dd7eee8719435e87b786dfb43d089e73561a33ce842613a2fc7a3894bea959R968

However relying on path.dirname causes problems with setting the current working directory for the interactive window and I would argue that we should always be checking whether the launchingFile is valid, since otherwise it's possible for us to end up in states like this: #6549 (comment)

Do you foresee any issues with this change?

@joyceerhl joyceerhl requested a review from a team as a code owner July 7, 2021 23:22
@codecov-commenter
Copy link

Codecov Report

Merging #6577 (93f4e28) into main (f287670) will increase coverage by 0%.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main   #6577   +/-   ##
=====================================
  Coverage     69%     69%           
=====================================
  Files        410     410           
  Lines      28243   28240    -3     
  Branches    4193    4196    +3     
=====================================
+ Hits       19632   19702   +70     
+ Misses      6960    6862   -98     
- Partials    1651    1676   +25     
Impacted Files Coverage Δ
src/client/datascience/jupyter/jupyterNotebook.ts 76% <ø> (-1%) ⬇️
src/client/datascience/notebook/introStartPage.ts 58% <0%> (-42%) ⬇️
src/client/datascience/jupyter/jupyterSession.ts 74% <0%> (-10%) ⬇️
...t/datascience/kernel-launcher/localKernelFinder.ts 84% <0%> (-6%) ⬇️
src/client/common/process/pythonDaemonPool.ts 75% <0%> (-5%) ⬇️
.../datascience/kernel-launcher/remoteKernelFinder.ts 90% <0%> (-4%) ⬇️
...rc/client/common/process/pythonExecutionFactory.ts 82% <0%> (-3%) ⬇️
...ent/datascience/jupyter/pythonVariableRequester.ts 72% <0%> (-2%) ⬇️
.../datascience/notebook/notebookControllerManager.ts 81% <0%> (-2%) ⬇️
...ent/datascience/jupyter/kernels/kernelExecution.ts 67% <0%> (-2%) ⬇️
... and 24 more

@joyceerhl joyceerhl merged commit 14d5a86 into main Jul 8, 2021
@joyceerhl joyceerhl deleted the dev/joyceerhl/iw-pwd branch July 8, 2021 01:47
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