Improve interpreter selection on different platforms #517

MikhailArkhipov · 2018-01-02T17:27:52Z

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

DonJayamanne · 2018-01-02T17:49:18Z

src/client/common/installer/pythonInstallation.ts

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


What if platform is Linux?

I think it should be like Windows then

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

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

DonJayamanne · 2018-01-02T17:52:19Z

src/client/common/platform/platformService.ts

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


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

Good catch...

DonJayamanne · 2018-01-02T17:55:20Z

src/test/index.ts

@@ -13,7 +13,8 @@ const options: MochaSetupOptions & { retries: number } = {
    ui: 'tdd',
    useColors: true,
    timeout: 25000,
-    retries: 3
+    retries: 3,
+    grep: 'Signatures'


DonJayamanne · 2018-01-02T23:40:05Z

src/client/extension.ts

@@ -96,7 +97,7 @@ export async function activate(context: vscode.ExtensionContext) {
    const interpreterManager = new InterpreterManager(serviceContainer);

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


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)

DonJayamanne · 2018-01-02T23:47:23Z

src/test/index.ts

@@ -14,7 +14,7 @@ const options: MochaSetupOptions & { retries: number } = {
    useColors: true,
    timeout: 25000,
    retries: 3,
-    grep: 'Signatures'
+    grep: 'Installation'


Please remove this.

DonJayamanne · 2018-01-02T23:49:18Z

src/client/common/installer/pythonInstallation.ts

        if (this.platform.isMac) {
-            if (await this.shell.showErrorMessage('Python that comes with Mac OS is not supported. Would you like to install regular Python now?', 'Yes', 'No') === 'Yes') {
+            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');


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

DonJayamanne · 2018-01-02T23:52:36Z

src/client/common/installer/pythonInstallation.ts

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


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.

DonJayamanne · 2018-01-02T23:53:27Z

src/client/common/platform/fileSystem.ts

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


please use path.sep instead

DonJayamanne · 2018-01-02T23:55:03Z

src/client/common/platform/fileSystem.ts

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


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.

DonJayamanne · 2018-01-02T23:57:07Z

src/client/common/platform/fileSystem.ts

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


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

DonJayamanne · 2018-01-03T00:06:03Z

src/client/common/installer/pythonInstallation.ts

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


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

DonJayamanne · 2018-01-03T00:09:44Z

src/client/extension.ts


    const persistentStateFactory = serviceManager.get<IPersistentStateFactory>(IPersistentStateFactory);
    const pythonSettings = settings.PythonSettings.getInstance();
    sendStartupTelemetry(activated, serviceContainer);

    sortImports.activate(context, standardOutputChannel);
    const interpreterManager = new InterpreterManager(serviceContainer);
+
+    const pythonInstaller = new PythonInstaller(serviceContainer);
+    const passed = await pythonInstaller.checkPythonInstallation(PythonSettings.getInstance());


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 @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 @qubitron to comment on this.

DonJayamanne · 2018-01-03T00:12:34Z

src/test/index.ts

@@ -13,7 +13,8 @@ const options: MochaSetupOptions & { retries: number } = {
    ui: 'tdd',
    useColors: true,
    timeout: 25000,
-    retries: 3
+    retries: 3,
+    grep: 'Installation'


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

MikhailArkhipov mentioned this pull request Jan 2, 2018

Intellisense hangs with Loading... #423

Closed

DonJayamanne mentioned this pull request Jan 2, 2018

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

Closed

DonJayamanne requested changes Jan 2, 2018

View reviewed changes

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

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

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

MikhailArkhipov merged commit 4372809 into microsoft:master Jan 4, 2018

MikhailArkhipov deleted the inst1 branch January 4, 2018 00:27

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve interpreter selection on different platforms #517

Improve interpreter selection on different platforms #517

MikhailArkhipov commented Jan 2, 2018

DonJayamanne Jan 2, 2018

MikhailArkhipov Jan 2, 2018

DonJayamanne Jan 3, 2018

MikhailArkhipov Jan 3, 2018

DonJayamanne Jan 2, 2018

MikhailArkhipov Jan 2, 2018

DonJayamanne Jan 2, 2018

DonJayamanne Jan 2, 2018

DonJayamanne Jan 2, 2018

DonJayamanne Jan 2, 2018

DonJayamanne Jan 2, 2018

DonJayamanne Jan 2, 2018

MikhailArkhipov Jan 3, 2018

DonJayamanne Jan 2, 2018

MikhailArkhipov Jan 3, 2018

DonJayamanne Jan 2, 2018

MikhailArkhipov Jan 3, 2018

DonJayamanne Jan 3, 2018

DonJayamanne Jan 2, 2018

DonJayamanne Jan 3, 2018

DonJayamanne Jan 3, 2018

DonJayamanne Jan 3, 2018

MikhailArkhipov Jan 3, 2018

DonJayamanne Jan 3, 2018

DonJayamanne Jan 3, 2018

Improve interpreter selection on different platforms #517

Improve interpreter selection on different platforms #517

Conversation

MikhailArkhipov commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment