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

Add Pylance setting and prompt for install if Pylance extension is missing #13123

Merged
merged 36 commits into from
Jul 29, 2020
Merged

Add Pylance setting and prompt for install if Pylance extension is missing #13123

merged 36 commits into from
Jul 29, 2020

Conversation

MikhailArkhipov
Copy link

For #13122

  • Add Pylance setting to python.languageServer

  • Check if Pylance is installed and prompt for install if it is missing

  • Related tests

  • Add few strings to localization

  • 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-commenter
Copy link

codecov-commenter commented Jul 23, 2020

Codecov Report

Merging #13123 into master will decrease coverage by 0.16%.
The diff coverage is 92.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13123      +/-   ##
==========================================
- Coverage   59.79%   59.62%   -0.17%     
==========================================
  Files         671      670       -1     
  Lines       36681    36835     +154     
  Branches     5158     5189      +31     
==========================================
+ Hits        21933    21963      +30     
- Misses      13655    13777     +122     
- Partials     1093     1095       +2     
Impacted Files Coverage Δ
src/client/activation/common/activatorBase.ts 13.22% <ø> (-9.00%) ⬇️
...t/activation/common/languageServerFolderService.ts 98.36% <ø> (ø)
src/client/activation/node/languageServerProxy.ts 26.58% <0.00%> (ø)
src/client/activation/node/manager.ts 26.78% <0.00%> (ø)
src/client/common/featureDeprecationManager.ts 40.90% <ø> (ø)
...active-common/intellisense/intellisenseProvider.ts 37.73% <ø> (+0.11%) ⬆️
src/client/providers/jediProxy.ts 23.59% <0.00%> (ø)
src/client/telemetry/constants.ts 100.00% <ø> (ø)
src/client/telemetry/index.ts 82.35% <ø> (ø)
src/client/activation/activationService.ts 89.72% <91.66%> (+5.57%) ⬆️
... and 90 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 6c99859...9e05807. Read the comment docs.

@MikhailArkhipov MikhailArkhipov marked this pull request as ready for review July 23, 2020 23:29
src/client/activation/node/activator.ts Outdated Show resolved Hide resolved
src/client/activation/node/activator.ts Outdated Show resolved Hide resolved
package.nls.json Outdated Show resolved Hide resolved
package.nls.json Outdated Show resolved Hide resolved
@jakebailey
Copy link
Member

One other thing that would be good to do:

Right now, for people without Pylance installed, it's going to be two reloads because the process is like:

  1. Change in the settings to Pylance. Extension prompts "LS changed, please reload".
  2. New Pylance code runs, and Pylance extension isn't installed. Extension prompts "go here to get Pylance".

Can the first step be modified to say "if changing to Pylance and Pylance not installed, load pylance banner instead"? That way you only have the one reload to actually finish the whole process.

@MikhailArkhipov
Copy link
Author

Basically activator and change settings tracker should be calling the same code that would check if Pylance is installed. So the code needs to move from activator into some shared class.

@jakebailey
Copy link
Member

Some of it, anyway. Three lines to do if (this.extensions.getExtension(PYLANCE_EXTENSION_ID) { ... } is probably not worth the abstraction, but the rest of the banner yeah.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Behavior looks good to me.

@sonarcloud
Copy link

sonarcloud bot commented Jul 29, 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 1 Code Smell

No Coverage information No Coverage information
0.9% 0.9% Duplication

@MikhailArkhipov MikhailArkhipov merged commit 5841782 into microsoft:master Jul 29, 2020
@MikhailArkhipov MikhailArkhipov deleted the settings branch July 30, 2020 22:27
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