[Fixes #2459] Do not use a variable substitution when updating python.pythonPath. #2685

ericsnowcurrently · 2018-09-25T20:01:26Z

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!)
Has unit tests & system/integration tests
~~Any new/changed dependencies in package.json are pinned (e.g. "1.2.3", not "^1.2.3" for the specified version)~~
~~package-lock.json has been regenerated by running npm install (if dependencies have changed)~~

DonJayamanne · 2018-09-25T21:24:02Z

src/client/interpreter/configuration/services/workspaceFolderUpdaterService.ts

@@ -1,22 +1,26 @@
-import * as path from 'path';
+// Copyright (c) Microsoft Corporation. All rights reserved.


Please remove the copyright, this is to be added only for new files.

DonJayamanne · 2018-09-25T21:26:06Z

src/client/interpreter/configuration/services/workspaceFolderUpdaterService.ts

    public async updatePythonPath(pythonPath: string): Promise<void> {
        const pythonConfig = this.workspaceService.getConfiguration('python', this.workspaceFolder);
        const pythonPathValue = pythonConfig.inspect<string>('pythonPath');

        if (pythonPathValue && pythonPathValue.workspaceFolderValue === pythonPath) {
            return;
        }
-        if (pythonPath.startsWith(this.workspaceFolder.fsPath)) {


We need to restore the old change, and add the relative path. Right now it will add the fully qualified path, this makes it less portable for teams.

The benefit of using ${workspaceFolder} in settings was teams could share the settings.json file as the paths are relative, now they are absolute making them less portable, this would be a breaking change.

The whole point of this PR is to remove the variable substitution. I'm not clear what alternative you are suggesting.

DonJayamanne · 2018-09-25T21:26:18Z

src/client/interpreter/configuration/services/workspaceUpdaterService.ts

    public async updatePythonPath(pythonPath: string): Promise<void> {
        const pythonConfig = this.workspaceService.getConfiguration('python', this.workspace);
        const pythonPathValue = pythonConfig.inspect<string>('pythonPath');

        if (pythonPathValue && pythonPathValue.workspaceValue === pythonPath) {
            return;
        }
-        if (pythonPath.startsWith(this.workspace.fsPath)) {


Please see previous comment.

DonJayamanne · 2018-09-25T21:26:25Z

src/client/interpreter/configuration/services/workspaceUpdaterService.ts

@@ -1,22 +1,26 @@
-import * as path from 'path';
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.


Same as previous comment.

DonJayamanne · 2018-09-25T21:27:34Z

src/test/interpreter/configuration/services/workspaceUpdaterService.unit.test.ts

+    if (filename === '') {
+        return '';
+    }
+    const parts = filename.split('/');


Do not hardccode file path seprators. On Windows its \ and others its /

In tests it's much more readable to do it the way I've done it with little cost. The point of this helper function is to keep the tests cross-platform while keeping the test code more readable.

The fact that you are splitting with '/' doesn't make it cross platform.
On windows path separators are \.

The point is that the tests define everything as if on linux and then normalizeFilename() fixes up the filenames to be cross-platform. That makes the tests more readable and easier to write.

DonJayamanne · 2018-09-25T21:33:54Z

src/test/interpreter/configuration/services/workspaceUpdaterService.unit.test.ts

+        ['absolute - starts with workspace root', '/home/user/project', '/home/user/project/my-venv/bin/python3', ''],
+        ['absolute - does not start with workspace root', '/home/user/project', '/home/user/my-venv/bin/python3', '']
+    ];
+    for (let [testName, workspaceRoot, pythonPath, expected] of tests) {


In all of the items in the array, the 4th item is always '', meaning, expected is always empty, hence I do not see the purpose of the normalizeFilename.
Also why can't we write expected file name in the test rather than computing it, more specific.

I made '' mean "use pythonPath solely so the array is less cluttered and to make it extra clear that pythonPath does not get changed.

Don't understand you, either way, why can't we remove it!
It is ALWAYS empty, hence see no point in having more code to check if its empty or not, when it is.

It's not always empty though. :)

DonJayamanne · 2018-09-25T21:34:33Z

src/test/interpreter/configuration/services/workspaceUpdaterService.unit.test.ts

+        if (expected === '') {
+            expected = pythonPath;
+        } else {
+            expected = normalizeFilename(expected);


This (else) code will never get executed.

Sure, with the current test cases it won't. However, I anticipated that there would be other test cases that would change the path, hence accommodating separate "expected" paths in the tests.

I'd say this is YAGNI, lets not plan for the future, its easy to add it then, hence lets remove dead code now.

Except I just had to add tests that used it. :)

Personally I don't see the point of having normalizeFilename.
We can easily hard code the strings as you have already done in the array.

I'm looking at this code, and I cannot understand why we cannot just hardcode the expected value as the 4th item in the index, that increases readability.
Right now, one has to follow the code, look at why its empty, then check the if condition, then follow and understand what normalizeFileName does, all to understand what the expected name is!!

Please remove tests for CWD, its impossible scenario.
I.e. we cannot predict outcome for an impossible scenario.
I didn't bring this up earlier as i assumed this was the workspace folder, now understand this based on our conversation from earlier today.

Now, looking at the revised code, I personally don't see the need for new unit test file either, we should be able to write 2 new tests in the old test file that will account for non-relative paths.

I didn't raise this as the code had to be refactored to bring back the old code, now that the old functionality has been brought back to store relative paths, this makes testing easier.

Also the other tests are much simpler than this (this feels more complicated with normalize... and its hardcoded for unix paths).

Summary:

Add tests for pythonpath = 'python3'

Add tests for pythonpath = 'home/usr/dev/env/bin/python'
(make use of path.join(...) instead of hard coding /.
This should simplify the tests.

You should be able to use the same concept of using a loop with an array of items containing the data and the expectations.

DonJayamanne · 2018-09-25T21:38:01Z

src/test/interpreter/configuration/services/workspaceFolderUpdaterService.unit.test.ts

+    WorkspaceFolderPythonPathUpdaterService
+} from '../../../../client/interpreter/configuration/services/workspaceFolderUpdaterService';
+
+function normalizeFilename(filename: string): string {


Not sure what this is doing, either way we have code duplicated.
Please try to use path.normalize.
Also, please do not hardcode /, this helps with cross platform tests, specifically windows.

See other comments about this.

DonJayamanne · 2018-09-25T21:38:26Z

src/test/interpreter/configuration/services/workspaceFolderUpdaterService.unit.test.ts

+        if (expected === '') {
+            expected = pythonPath;
+        } else {
+            expected = normalizeFilename(expected);


See comment in next file, this line of code will never get executed.

DonJayamanne · 2018-09-25T21:39:46Z

src/test/interpreter/configuration/services/workspaceFolderUpdaterService.unit.test.ts

+
+    const tests: [string, string, string, string][] = [
+        // (test name, root, new pythonPath, expected)
+        ['on $PATH', 'project', 'python3', ''],


Just as in other file, why is the 4th argument computed. Why can't we hard code it here e.g. ['on $PATH', 'project', 'python3', 'python3'],
['under $HOME', 'project', '~/my-venv/bin/python3', '~/my-venv/bin/python3'],
['relative -- under workspace root', 'project', 'project/my-venv/bin/python3', 'my-venv/bin/python3'],

DonJayamanne · 2018-09-26T01:27:42Z

src/test/interpreter/configuration/services/workspaceUpdaterService.unit.test.ts

+        if (expected === '') {
+            expected = pythonPath;
+        } else {
+            expected = normalizeFilename(expected);


Personally I don't see the point of having normalizeFilename.
We can easily hard code the strings as you have already done in the array.

I'm looking at this code, and I cannot understand why we cannot just hardcode the expected value as the 4th item in the index, that increases readability.
Right now, one has to follow the code, look at why its empty, then check the if condition, then follow and understand what normalizeFileName does, all to understand what the expected name is!!

DonJayamanne · 2018-09-26T01:29:57Z

src/test/interpreter/configuration/services/workspaceUpdaterService.unit.test.ts

+    WorkspacePythonPathUpdaterService
+} from '../../../../client/interpreter/configuration/services/workspaceUpdaterService';
+
+function normalizeFilename(filename: string): string {


Then what are we doing? I just don't see the purpose of this, when we can hard code the expected value in the array, doing that makes it completely unnecessary to write more code in the tests, and makes it a whole lot easier to read and understand.

DonJayamanne · 2018-09-26T01:45:26Z

src/test/interpreter/configuration/services/workspaceUpdaterService.unit.test.ts

+        if (expected === '') {
+            expected = pythonPath;
+        } else {
+            expected = normalizeFilename(expected);


Please remove tests for CWD, its impossible scenario.
I.e. we cannot predict outcome for an impossible scenario.
I didn't bring this up earlier as i assumed this was the workspace folder, now understand this based on our conversation from earlier today.

Now, looking at the revised code, I personally don't see the need for new unit test file either, we should be able to write 2 new tests in the old test file that will account for non-relative paths.

I didn't raise this as the code had to be refactored to bring back the old code, now that the old functionality has been brought back to store relative paths, this makes testing easier.

Also the other tests are much simpler than this (this feels more complicated with normalize... and its hardcoded for unix paths).

Summary:

Add tests for pythonpath = 'python3'

Add tests for pythonpath = 'home/usr/dev/env/bin/python'
(make use of path.join(...) instead of hard coding /.
This should simplify the tests.

You should be able to use the same concept of using a loop with an array of items containing the data and the expectations.

ericsnowcurrently · 2018-09-26T22:13:45Z

I've simplified things by factoring out a base class for the two *Service classes. I've also added more tests and dropped normalizeFilename(). FWIW, I still find the composed (via path.join()) filenames less readable, but this isn't important enough to block on. :)

DonJayamanne · 2018-09-26T22:47:24Z

Why the whole refactor for this simple change? Initially nothing much changed, now you have refactored a lot more.

The previous code was much simpler, we now have new API that's really unnecessary.

DonJayamanne · 2018-09-26T22:52:53Z

src/client/common/configSettings.ts

@@ -28,6 +28,41 @@ const untildify = require('untildify');

 export const IS_WINDOWS = /^win/.test(process.platform);

+export function getSetting<T>(


This function is unnecessary.

More code to be tested, this is basically a wrapper for a one-line code in VSC.

DonJayamanne · 2018-09-26T23:52:25Z

Please review my PR and the changes #2705
Lets discuss tomorrow.

The new function getSetting you introduced basically replaces the following code:

const pythonPathValue = pythonConfig.inspect<string>('pythonPath');	
if (pythonPathValue && pythonPathValue.workspaceValue === pythonPath) {
    return;
}

DonJayamanne · 2018-09-26T23:17:14Z

src/client/common/configSettings.ts

@@ -28,6 +28,41 @@ const untildify = require('untildify');

 export const IS_WINDOWS = /^win/.test(process.platform);

+export function getSetting<T>(


More code to be tested, this is basically a wrapper for a one-line code in VSC.

ericsnowcurrently · 2018-09-27T16:17:56Z

I'm fine with just landing the minimal change. The point of all the extra code is to simplify things and add unit test coverage, both of which are worth doing. I considered putting that extra stuff in a separate PR, but figured it wasn't that big a deal. So in the interest of getting the actual fix landed I'll punt on that.

FWIW, I'd like to discuss your objections separately (not here or now) as I have a different perspective on the value of the extra changes.

…onPath().

DonJayamanne requested changes Sep 25, 2018

View reviewed changes

DonJayamanne requested changes Sep 26, 2018

View reviewed changes

ericsnowcurrently added 16 commits September 27, 2018 10:45

Add unit tests for WorkspacePythonPathUpdaterService.updatePythonPath().

53776a6

Parameterize some of the tests.

aa5fecd

Factor out setExpected().

0f1359d

Add unit tests for WorkspaceFolderPythonPathUpdaterService.updatePyth…

5adde4e

…onPath().

Do not set python.pythonPath to a value with variable substitution.

921e092

Add a NEWS entry.

568a5dd

Drop two system tests that expect the old behavior.

42f697b

Do not add copyright to existing files.

afa83a3

Keep pythonPath relative when possible.

7761615

Combine the workspace updaters into one file.

2d16f10

Add getSetting() and matchSetting() helpers.

4f14402

Use matchSetting().

269982b

Factor out WorkspacePythonPathUpdater.

b296399

Drop normalizeFilename().

40d47ec

Fix some unrelated tests.

2fd344e

Drop the refactoring and extra unit tests.

3e7923d

ericsnowcurrently force-pushed the fix-2459-settings-workspaceFolder branch from 300e67c to 3e7923d Compare September 27, 2018 17:02

DonJayamanne approved these changes Sep 27, 2018

View reviewed changes

ericsnowcurrently merged commit 5e92628 into microsoft:master Sep 27, 2018

ericsnowcurrently deleted the fix-2459-settings-workspaceFolder branch September 27, 2018 18:17

ericsnowcurrently mentioned this pull request Sep 27, 2018

Clean up python path updaters and add more unit tests. #2715

Closed

tt4g mentioned this pull request Nov 24, 2018

support relative python.pythonPath vscode-restructuredtext/vscode-restructuredtext#122

Closed

6 tasks

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

		@@ -1,22 +1,26 @@
		import * as path from 'path';
		// Copyright (c) Microsoft Corporation. All rights reserved.

		@@ -28,6 +28,41 @@ const untildify = require('untildify');

		export const IS_WINDOWS = /^win/.test(process.platform);

		export function getSetting<T>(

[Fixes #2459] Do not use a variable substitution when updating python.pythonPath. #2685

[Fixes #2459] Do not use a variable substitution when updating python.pythonPath. #2685

Conversation

ericsnowcurrently commented Sep 25, 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

DonJayamanne Sep 25, 2018 • edited Loading

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

ericsnowcurrently commented Sep 26, 2018

DonJayamanne commented Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne commented Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

ericsnowcurrently commented Sep 27, 2018

DonJayamanne Sep 25, 2018 •

edited

Loading

DonJayamanne commented Sep 26, 2018 •

edited

Loading

DonJayamanne commented Sep 26, 2018 •

edited

Loading