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

Leave pipenv in a corner until the user decides to select an interpreter #11654

Merged

Conversation

kimadeline
Copy link

@kimadeline kimadeline commented May 6, 2020

For #11127

Instead of tracking all the places that call getInterpreters on activation, and passing { onActivation: true} 15 levels deep, here's the new plan:

  • remove:
    • the onActivation option
    • the registration of the IPipEnvService singleton
  • add an onSuggestion option that will be true when the user clicks on the interpreter in the bottom-left corner or runs the select interpreter command. This will set a didTriggerSuggestions boolean to true the first time an interpreter is selected
  • if didTriggerSuggestions is true -> set it to true inside the locators as well
  • whenever a method from the pipenv locator is executed -> exit early if didTriggerSuggestions is not true

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

@kimadeline kimadeline added the no-changelog No news entry required label May 6, 2020
@kimadeline kimadeline marked this pull request as ready for review May 7, 2020 02:33
@codecov-io
Copy link

codecov-io commented May 7, 2020

Codecov Report

Merging #11654 into master will increase coverage by 0.21%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11654      +/-   ##
==========================================
+ Coverage   60.70%   60.91%   +0.21%     
==========================================
  Files         626      632       +6     
  Lines       33767    34237     +470     
  Branches     4748     4845      +97     
==========================================
+ Hits        20498    20857     +359     
- Misses      12289    12369      +80     
- Partials      980     1011      +31     
Impacted Files Coverage Δ
src/client/interpreter/contracts.ts 100.00% <ø> (ø)
src/client/interpreter/serviceRegistry.ts 100.00% <ø> (ø)
src/client/startupTelemetry.ts 30.00% <0.00%> (ø)
...reter/locators/services/cacheableLocatorService.ts 75.22% <20.00%> (-2.56%) ⬇️
...ent/interpreter/locators/services/pipEnvService.ts 83.63% <57.14%> (-0.98%) ⬇️
src/client/activation/activationManager.ts 95.37% <100.00%> (ø)
...ication/diagnostics/checks/macPythonInterpreter.ts 84.84% <100.00%> (ø)
...entActivationProviders/pipEnvActivationProvider.ts 64.28% <100.00%> (ø)
...c/client/interpreter/autoSelection/rules/system.ts 100.00% <100.00%> (ø)
...uration/interpreterSelector/interpreterSelector.ts 85.18% <100.00%> (ø)
... and 38 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 f4c1ac3...1c5c0a2. 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.

I noticed the class PipEnvService is being registered twice.

    serviceManager.addSingleton<IInterpreterLocatorService>(IInterpreterLocatorService, PipEnvService, PIPENV_SERVICE);
    serviceManager.addSingleton<IInterpreterLocatorService>(IPipEnvService, PipEnvService);

You're making sure that it doesn't get invoked using IInterpreterLocatorService, but it can still get invoked when IPipEnvService is injected somewhere (for instance)

Please remove the redundant registration using IPipEnvService and replace it with IInterpreterLocatorService.

@kimadeline kimadeline requested a review from karrtikr May 7, 2020 22:24
@sonarcloud
Copy link

sonarcloud bot commented May 8, 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

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

@kimadeline kimadeline merged commit 7984076 into microsoft:master May 8, 2020
@kimadeline kimadeline deleted the 11127-pipenv-existence-activation branch May 8, 2020 14:38
karthiknadig pushed a commit to karthiknadig/vscode-python that referenced this pull request May 8, 2020
…ter (microsoft#11654)

* add onSuggestion option
* Swap onActivation with onSuggestion
* Update unit tests
* Remove registration of IPipenvService
* Move didTriggerInterpreterSuggestions logic inside pipenv locator
* Fix existing unit tests
* Add new unit tests
* Replace typemoq any param with object
* Shorten the tests
* Fix warning
* Duplicate teardown
karthiknadig added a commit that referenced this pull request May 8, 2020
…11700)

* Show the prompt again if user clicks on more info (#11664)

* Show the prompt again if user clicks on more info

* Review feedback

* Use Learn more as text for the link.

* Leave pipenv in a corner until the user decides to select an interpreter (#11654)

* add onSuggestion option
* Swap onActivation with onSuggestion
* Update unit tests
* Remove registration of IPipenvService
* Move didTriggerInterpreterSuggestions logic inside pipenv locator
* Fix existing unit tests
* Add new unit tests
* Replace typemoq any param with object
* Shorten the tests
* Fix warning
* Duplicate teardown

Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com>
karthiknadig added a commit that referenced this pull request May 19, 2020
* Revert vscode-extension-telemetry changes for the release (#11602) (#11656)

* Revert "Fix slashes in telemetry unit tests (#11572)"

This reverts commit 7431c9c.

* Revert "Use vscode-extension-telemetry for our exceptions & error telemetry (#11524)"

This reverts commit d5065e6.

* Remove from changelog

* Port storage fix to release branch (#11673)

* Fix storage not being used (#11649)

* Fix storage not being used

* Add disposable to storage so it won't write after shutdown

* Fix dirty title

* Hack to get tests to pass

* Another way to get run all to not interfere

* Update changelog

* Port scrolling fix to release (#11688)

* Fix scrolling (#11681)

* Fix scrolling

* Review feedback - fix scrolling on expand/collapse

* Update changelog

* Update package.json

Co-authored-by: Jim Griesmer <jimgries@microsoft.com>

* Cherry-pick pipenv changes and pythonpath prompt changes to release (#11700)

* Show the prompt again if user clicks on more info (#11664)

* Show the prompt again if user clicks on more info

* Review feedback

* Use Learn more as text for the link.

* Leave pipenv in a corner until the user decides to select an interpreter (#11654)

* add onSuggestion option
* Swap onActivation with onSuggestion
* Update unit tests
* Remove registration of IPipenvService
* Move didTriggerInterpreterSuggestions logic inside pipenv locator
* Fix existing unit tests
* Add new unit tests
* Replace typemoq any param with object
* Shorten the tests
* Fix warning
* Duplicate teardown

Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com>

* Update extension version (#11730)

* Update extension version

* Update date in changelog.

* Update change log with additional notes. (#11764)

* Cherry picks and version updates for bug fix release (#11878)

* Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang (#11816)

* Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang

* Rename

* Oops

* Update src/test/providers/shebangCodeLenseProvider.unit.test.ts

Co-authored-by: Karthik Nadig <kanadig@microsoft.com>

Co-authored-by: Karthik Nadig <kanadig@microsoft.com>

* Update version and change log for bugfix release

Co-authored-by: Kartik Raj <karraj@microsoft.com>

* Clean up news

Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com>
Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com>
Co-authored-by: Jim Griesmer <jimgries@microsoft.com>
Co-authored-by: Kartik Raj <karraj@microsoft.com>
@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
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants