Improve interpreter selection on different platforms by MikhailArkhipov · Pull Request #517 · microsoft/vscode-python · GitHub

Preliminary , working on tests

Open Python installation page on Windows if Python is missing
Exclude default Apple Python on Mac
Install HomeBrew and Python on Mac if only system Python is present
Improve user messaging
Introduce abstracted app shell (showErrorMessage etc and file system for testability

…m list

…nto inst1

What if platform is Linux?

I think it should be like Windows then

I think linux too has a version of Python pre-installed. Brett Cannon (@brettcannon) Do you think we should support the system python in linux?

Or we can leave Linux alone and let users figure it out?

Isn't the condition supposed to be return !(this.isWindows || this.isMac);

Good catch...

Booboo

On second thought this might not be the best approach.
Users will now get bombarded with messages every time they open a python file in a new workspace.

Previously we'd display a warning on the bottom status bar. This change causes the display of a message every time you open a new workspace.

E.g. people using VS Code for scripting purposes or the like would now see this message every time, and we'd end up with the same issue we had with the linter. The solution was to add an option to stop showing the message ever again.

My suggestion is to perform this check when installing a linter or something similar. I.e. at a point in time when the user has been presented with some UI (e.g. selecting an interperter, setting up tests, installing linter, etc from the extension)

Please remove this.

How about including instructions for the user to install Brew package manager. E.g.
Unable to install Brew package manager, please install Brew package manager and try again

Wouldn't it be better to mock the process, without having to add conditional test code. Feels a little dirty and I'm afraid we'd end up writing a lot more of this.

Yeah. I tried that but mocking of events is pretty convoluted. Basically something needs to emit exit asynchronously while test is running.

please use path.sep instead

OK

Please use fs.pathExists (use import * as fs from 'fs-extra').
As a suggestion, wouldn't it be better to have fileExists and directoryExists.
I have a need for this (have already create similar methods in another PR, where I want to differentiate between files and directories)

Sure, I can add dir/file separately. We can add more as we go. I didn't want to add all possible combos.

Yes sure.

you can use fs.mkdirp. I wouldn't swallow the exceptions, just use return fs.mkdirp(directoryPath)

I think linux too has a version of Python pre-installed. Brett Cannon (@brettcannon) Do you think we should support the system python in linux?

I don't think this is a good idea. Every time a Mac user opens a workspace or a python file in a new workspace, they have the potential of being hammered with a message. This could end up being an annoying message we had with the linters, the solution to which was to hide the message permanently.

Currently we display a warning in the status bar if the instance of python is invalid (none selected). Personally i prefer that (having learnt the lesson from the linter message). We could display a message when the user tries to install a linter, or setup unit tests or similar.

I'd say this is where Dan Taylor (@qubitron) would have to get involved as this is sort of a UX thing and we've had problems in the past (annoying linter messages) which was conveyed in the survey.

Add Don't show this again?

I'd wait for Dan Taylor (@qubitron) to comment on this.

Please remove

…nto inst1

* upstream/master: Improve interpreter selection on different platforms (#517) Yarn and code coverage (#475)

MikhailArkhipov added 30 commits December 1, 2017 10:18

Basic tokenizer

7675901

Fixed property names

eb42669

Tests, round I

2756974

Tests, round II

c2c1ced

merge master

a108c96

tokenizer test

14864a5

Remove temorary change

0ed51d6

Fix merge issue

51b544c

Merge conflict

3cd11e6

Merge conflict

82e0ad1

Completion test

9295c1a

Fix last line

06eb1a5

Fix javascript math

e9db8e0

Merge master

d12ca03

Make test await for results

d8ab041

Add license headers

db75cd0

Rename definitions to types

9ab2c47

License headers

d587485

Merge branch 'master' of https://github.com/Microsoft/vscode-python

1da5e0a

Merge branch 'master' of https://github.com/Microsoft/vscode-python

7668cee

Fix typo in completion details (typo)

1ac4932

Fix hover test

2aa5a6c

Merge branch 'master' of https://github.com/Microsoft/vscode-python

5db31bd

Russian translations

560d2af

Merge branch 'master' of https://github.com/Microsoft/vscode-python

c71024d

Update to better translation

31aa087

Fix typo

593ae05

#70 How to get all parameter info when filling in a function param list

e6d69bb

Fix #70 How to get all parameter info when filling in a function para…

b5a23d3

…m list

Clean up

cd200f7

Mikhail Arkhipov added 8 commits December 15, 2017 13:55

Merge branch 'master' of https://github.com/Microsoft/vscode-python

bfcae78

Merge branch 'master' of https://github.com/Microsoft/vscode-python

42a5f79

Basic services

699c434

Install check

dd9ba0a

Output installer messages

443ad65

Warn default Mac OS interpreter

fdde6b8

Merge branch 'master' of https://github.com/Microsoft/vscode-python i…

e8ea50a

…nto inst1

Remove test change

8b7c920

Mikhail Arkhipov (MikhailArkhipov) mentioned this pull request Jan 2, 2018

Intellisense hangs with Loading... #423

Closed

Don Jayamanne (DonJayamanne) mentioned this pull request Jan 2, 2018

new npm packages added to package.json (Jan Release) #438

Closed

Don Jayamanne (DonJayamanne) requested changes Jan 2, 2018

View reviewed changes

Don Jayamanne (DonJayamanne) mentioned this pull request Jan 2, 2018

Document deprecation of support for system python on Mac #518

Closed

MikhailArkhipov added 2 commits January 2, 2018 15:17

Add tests

253df9e

PR feedback

97ed0a8

Don Jayamanne (DonJayamanne) requested changes Jan 3, 2018

View reviewed changes

MikhailArkhipov added 5 commits January 2, 2018 16:47

CR feedback

dcfc939

Mock process instead

34790bb

Fix Brew detection

b6caacc

Update test

bb648d7

Add check suppression option & suppress vor VE by default

d223410

Don Jayamanne (DonJayamanne) approved these changes Jan 3, 2018

View reviewed changes

MikhailArkhipov added 4 commits January 3, 2018 14:46

Merge branch 'master' of https://github.com/Microsoft/vscode-python i…

6a92e60

…nto inst1

Fix most linter tests

6873309

Merge conflict

9bd8948

Merge branch 'master' of https://github.com/Microsoft/vscode-python i…

11c3272

…nto inst1

Mikhail Arkhipov (MikhailArkhipov) merged commit 4372809 into microsoft:master Jan 4, 2018

Mikhail Arkhipov (MikhailArkhipov) deleted the inst1 branch January 4, 2018 00:27

Don Jayamanne (DonJayamanne) added a commit that referenced this pull request Jan 9, 2018

Merge remote-tracking branch 'upstream/master'

a31e659

* upstream/master: Improve interpreter selection on different platforms (#517) Yarn and code coverage (#475)

lock Bot locked as resolved and limited conversation to collaborators Jul 31, 2019

                   timeout: 25000,
-                  retries: 3
+                  retries: 3,
+                  grep: 'Signatures'

                   timeout: 25000,
                   retries: 3,
-                  grep: 'Signatures'
+                  grep: 'Installation'

+                          if (await this.shell.showErrorMessage('Python that comes with MacOS is not supported. Would you like to install regular Python now?', 'Yes', 'No') === 'Yes') {
                               const brewInstalled = await this.ensureBrew();
                               if (!brewInstalled) {
                                   await this.shell.showErrorMessage('Unable to install Brew package manager');

                   timeout: 25000,
-                  retries: 3
+                  retries: 3,
+                  grep: 'Installation'

+                              await this.executeAndOutput('brew', ['install', 'python']);
+                          }
+                      }

+                      return this._isMac;
+                  }
+                  public get isLinux(): boolean {
+                      return !(this.isWindows || this.isLinux);

                   const pythonInstaller = new PythonInstaller(serviceContainer);
-                  const passed = await pythonInstaller.checkPythonInstallation();
+                  const passed = await pythonInstaller.checkPythonInstallation(PythonSettings.getInstance());

+                          if (failed) {
+                              resolve(false);
+                          }
+                          if (isTestExecution()) {

+                  constructor( @inject(IServiceContainer) private platformService: IPlatformService) { }
+                  public get directorySeparatorChar(): string {
+                      return this.platformService.isWindows ? '\\' : '/';

+                  }
+                  public existsAsync(filePath: string): Promise<boolean> {
+                      return new Promise<boolean>(resolve => {

Improve interpreter selection on different platforms by MikhailArkhipov · Pull Request #517 · microsoft/vscode-python · GitHub

Uh oh!

Improve interpreter selection on different platforms - #517

Improve interpreter selection on different platforms#517Mikhail Arkhipov (MikhailArkhipov) merged 55 commits intomicrosoft:mastermicrosoft/vscode-python:masterfrom MikhailArkhipov:inst1Copy head branch name to clipboard

Conversation

Mikhail Arkhipov (MikhailArkhipov) commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Improve interpreter selection on different platforms#517
Mikhail Arkhipov (MikhailArkhipov) merged 55 commits into
microsoft:masterfrom
MikhailArkhipov:inst1