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

Change the logic for when we show the "Install Python (Extension)" commands #10584

Merged
merged 2 commits into from
Jun 27, 2022

Conversation

IanMatthewHuff
Copy link
Member

Fixes #10583

  • 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 feature-requests.
  • 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).

@IanMatthewHuff IanMatthewHuff requested a review from a team as a code owner June 27, 2022 09:26
@IanMatthewHuff
Copy link
Member Author

Per good suggestion from @rchiodo. The old dummy command logic for when it showed I just ported directly to the new commands, but with the new commands we can improve that logic some.

Old Logic:
No python connections detected - extension uninstalled => suggest extension
No python connections detected - extension installed => suggest python

New logic:
File is python or undefined - extension uninstalled => suggest extension
File is python or undefined - extension installed - no python connections detected => suggest python

Restricts it to python or unknown files. And if you happen to have a globally detected python kernel already on this system this will still provide the extension install command in the picker to suggest that you can install python to detect more kernels and for a better experience.

if (language && language !== this.lastSavedNotebookCellLanguage) {
await this.globalMemento.update(LastSavedNotebookCellLanguage, language);
}
}
private getLanguageOfFirstCodeCell(doc: NotebookDocument) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't need to be part of the class, and I needed it, so moved it out to a helper. Mild rename since the old name didn't seem quite accurate (it's more about getting the language of the document, not just the first cell as it prefers metadata over the first cell lang).

@codecov-commenter
Copy link

Codecov Report

Merging #10584 (bb0bb21) into main (521eda2) will increase coverage by 0%.
The diff coverage is 96%.

@@          Coverage Diff           @@
##            main   #10584   +/-   ##
======================================
  Coverage     71%      71%           
======================================
  Files        472      473    +1     
  Lines      28013    28025   +12     
  Branches    4697     4698    +1     
======================================
+ Hits       19943    19963   +20     
+ Misses      6196     6186   -10     
- Partials    1874     1876    +2     
Impacted Files Coverage Δ
src/notebooks/languages/helpers.ts 88% <88%> (ø)
...oks/controllers/installPythonControllerCommands.ts 59% <100%> (+9%) ⬆️
src/notebooks/languages/cellLanguageService.ts 70% <100%> (+<1%) ⬆️
src/kernels/variables/kernelVariables.ts 56% <0%> (-2%) ⬇️
...ebooks/controllers/notebookIPyWidgetCoordinator.ts 88% <0%> (-1%) ⬇️
...ctive-window/editor-integration/codeLensFactory.ts 88% <0%> (-1%) ⬇️
src/kernels/raw/launcher/kernelProcess.node.ts 70% <0%> (-1%) ⬇️
src/kernels/execution/kernelExecution.ts 70% <0%> (-1%) ⬇️
src/kernels/common/baseJupyterSession.ts 78% <0%> (-1%) ⬇️
src/kernels/types.ts 100% <0%> (ø)
... and 7 more

@IanMatthewHuff IanMatthewHuff merged commit 9b738e6 into main Jun 27, 2022
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/showInstallExtensionCommandMoreOften branch June 27, 2022 17:19
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.

Update logic to when we show the Install Python (Extension) commands
3 participants