Skip to content

Do not perform pipenv interpreter discovery on activation #11369

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

Conversation

kimadeline
Copy link

For #11127

  • 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).
  • The wiki is updated with any design decisions/details.

@codecov-io
Copy link

codecov-io commented Apr 23, 2020

Codecov Report

Merging #11369 into master will decrease coverage by 0.36%.
The diff coverage is 65.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11369      +/-   ##
==========================================
- Coverage   60.98%   60.61%   -0.37%     
==========================================
  Files         609      613       +4     
  Lines       33532    33502      -30     
  Branches     4742     4725      -17     
==========================================
- Hits        20448    20306     -142     
- Misses      12638    12745     +107     
- Partials      446      451       +5     
Impacted Files Coverage Δ
...client/datascience/kernel-launcher/kernelFinder.ts 60.71% <0.00%> (-0.55%) ⬇️
src/client/interpreter/contracts.ts 100.00% <ø> (ø)
src/client/startupTelemetry.ts 30.00% <0.00%> (ø)
...reter/locators/services/cacheableLocatorService.ts 77.77% <33.33%> (-0.93%) ⬇️
src/client/activation/activationManager.ts 95.37% <100.00%> (ø)
...ication/diagnostics/checks/macPythonInterpreter.ts 84.84% <100.00%> (ø)
...c/client/interpreter/autoSelection/rules/system.ts 100.00% <100.00%> (ø)
...nt/interpreter/autoSelection/rules/workspaceEnv.ts 96.07% <100.00%> (+1.34%) ⬆️
src/client/interpreter/interpreterService.ts 65.94% <100.00%> (ø)
src/client/interpreter/locators/index.ts 84.78% <100.00%> (ø)
... and 50 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 f96e762...d71790b. Read the comment docs.

Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

The file src\client\application\diagnostics\checks\macPythonInterpreter.ts also makes a call to IInterpreterService.getInterpreters, which happens during activation. Any reason for not passing the {onActivation: true} there?

@kimadeline
Copy link
Author

The file src\client\application\diagnostics\checks\macPythonInterpreter.ts also makes a call to IInterpreterService.getInterpreters, which happens during activation. Any reason for not passing the {onActivation: true} there?

Good catch 👍 I didn't hit this call when testing on my Mac because the diagnostic code bailed out early.

Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@karrtikr
Copy link

karrtikr commented Apr 29, 2020

@kimadeline I am assuming you'll be adding pipenv discovery on interpreter selection in some other PR, right?

We should probably merge it in a branch other than master, otherwise pipenv discovery will break for insiders users. What do you think?

@kimadeline
Copy link
Author

I am assuming you'll be adding pipenv discovery on interpreter selection in some other PR, right?

We already do that.

@karrtikr
Copy link

Ohk 👍 I saw the issue #11126 earlier which is why I was wondering.

@kimadeline
Copy link
Author

I updated my comment in #11126 to reflect that this was existing behaviour, so I didn't have to change anything.

@kimadeline kimadeline merged commit 486e3b3 into microsoft:master Apr 29, 2020
@kimadeline kimadeline deleted the 11126-pipenv-discovery-select-interpreter branch April 29, 2020 22:11
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants