Refactor extension to remove old way of spawning python processes #439

DonJayamanne · 2017-12-15T21:02:08Z

Fixes #354

Code refactoring to remove execPythonFile function and use the new execution layer

Archive of 0.7.0

* 'master' of https://github.com/Microsoft/vscode-python: Fixes #56 list all environments (#219) Fixes #57 Disable activation on debugging (#220) Fixes #26 Do not run linters when linters are disabled (#222)

* upstream/master: Fix typo in README.md (#252) Disable linter without workspaces (#241)

* upstream/master: Fix feedback service (#246) Fix django context initializer (#248) disable generation of tags file upon extension load (#264)

* upstream/master: Resolve pythonPath before comparing it to shebang (#273)

* upstream/master: Fixes #22 to Detect anaconda from known locations (#221) Use workspaceFolder token instead of workspaceRoot (#267) Fix registry lookup response (#224) Fix issues when running without debugging and debugged code terminates (#249)

* upstream/master: Fix debugging tests (#304)

* upstream/master: Remove jupyter functionality in favor of Jupyter extension (#302) Drop Python 2 URLs (#307)

* upstream/master: Remove setting python.formatting.formatOnSave in favor of the vs code setting (#312)

* upstream/master: Remove setting linting.lintOnTextChange as it was never implemented (#315)

* upstream/master: Fix travis build error (#326)

* upstream/master: add new npm deps with improved gulp for dev (#328)

* upstream/master: Update version of inversify package (#329)

* upstream/master: Document our dev process (#330)

* upstream/master: Document contribution to the code along with coding standards (#321)

* upstream/master: Add Simplified Chinese translation of commands (#240)

* upstream/master: Fix package.json (#347)

* upstream/master: #34, #110 - suppress Intellisense in strings and comments (#339) Re-factor code python execution framework (#345)

* upstream/master: Fix linters to make use of the new python code execution framework (#360) Update the versioning scheme (#356) Make npm happy in regards to line endings (#357)

* upstream/master: Ensure python path is not set if already set in user settings (#369) Use 'an' rather than 'a' before vowel words (#373)

* upstream/master: Use new environment variable parser (#362)

brettcannon · 2017-12-15T22:33:27Z

src/client/interpreter/interpreterVersion.ts

+        if (output.stdout.length > 0) {
+            // Take the first available version number, see below example.
+            // pip 9.0.1 from /Users/donjayamanne/anaconda3/lib/python3.6/site-packages (python 3.6).
+            // Take the second part, see below example.


What's the second part? And the example is the same, so it reads like you're taking two things from do different formats when it's two things from the same format.

Old comments, will fix that.

brettcannon · 2017-12-15T22:37:57Z

src/client/providers/jediProxy.ts

            } else {
-                args.push('custom');
-                args.push(this.pythonSettings.jediPath);
+                // Use preview version of jedi library.


I would drop "library" from the comment.

brettcannon · 2017-12-15T22:38:23Z

src/client/providers/jediProxy.ts

+            } else {
+                const data = output.out;
+                // Possible there was an exception in parsing the data returned,
+                // so append the data then parse it.


"data and then"

brettcannon · 2017-12-15T22:41:42Z

src/client/providers/jediProxy.ts

+                    // Great example of poorly written code (this whole file is a mess).
+                    // I think this needs to be removed, because this is misspelt, it is argments, 'U' is missing,
+                    // And that case is handled further down
+                    // case CommandType.Arguments: {


Uh, so I don't know what to do with this comment. 😄 If the code should be ripped out then rip it out and drop the comment. 😉

Can do, was reluctant to clean up this messy file.

brettcannon · 2017-12-15T22:42:19Z

src/client/providers/jediProxy.ts

+                        default:
+                            break;
+                    }
+                    // Check if too many pending requets.


DonJayamanne · 2017-12-15T23:39:00Z

@brettcannon all issues have been addressed. Thanks

DonJayamanne · 2017-12-19T00:45:13Z

@brettcannon all issues have been addressed. Thanks

* upstream/master: Refactor formatters to use new execution framework (#426)

brettcannon

Just a warning that my review was slightly rushed.

brettcannon · 2017-12-19T18:53:41Z

src/client/providers/jediProxy.ts

+                // tslint:disable-next-line:no-any
+                let responses: any[];
+                try {
+                    responses = dataStr.split(/\r?\n/g).filter(line => line.length > 0).map(resp => JSON.parse(resp));


Don't forget about that helper on splitting lines. 😉

DonJayamanne · 2017-12-19T19:09:53Z

@brettcannon Thats fine, I'll leave the PR open. Lets get it reviewed correctly in the new year. I'll base other PRs off this branch.

* upstream/master: Release 0.9.1 (#458) Add localization for Japanese (#434) Add Svn to uriSchemesToIgnore (#443)

brettcannon · 2018-01-02T20:56:09Z

src/client/interpreter/interpreterVersion.ts

+                return matches[0].trim();
+            }
+        }
+        throw new Error(`Unable to determine Pip version from output '${output.stdout}'`);


Technically pip is always lowercased.

brettcannon · 2018-01-02T20:59:47Z

src/client/providers/jediProxy.ts

+
+                    switch (cmd.command) {
+                        case CommandType.Completions:
+                            this.onCompletion(cmd, response);


Maybe assign the method to a variable and then call it after the switch to minimize the chance of getting the call order wrong?

brettcannon · 2018-01-02T21:05:37Z

src/test/autocomplete/pep484.test.ts

-        isPython3 = Promise.resolve(version.indexOf('3.') >= 0);
+        initializeDI();
+        const version = await ioc.getPythonVersion(rootWorkspaceUri);
+        isPython3 = version.indexOf('3.') >= 0;


This will break under Python 2.3 or Python 4.3. What you really want to know is whether something is Python 2 or not since Python 4 will be backwards-compatible with Python 3. IOW we want to set things up so that Python 2 is the special-case so when it comes time to drop support we can rip it out easily as well has not have issues when Python 4 is released.

👍 Agreed.

DonJayamanne · 2018-01-02T23:10:25Z

@brettcannon all done.

* upstream/master: Refactor parsing of environment variables (after 439) (#466) Refactor extension to remove old way of spawning python processes (#439)

* CommonToolExecution: Refactor parsing of environment variables (after 439) (#466) Refactor extension to remove old way of spawning python processes (#439)

octref and others added 30 commits November 3, 2017 13:11

Fix microsoft/vscode#37627 (#1368)

ecc1ca9

Version 0.7.0 of extension (#1381)

7c5778c

Archive of 0.7.0

Update README.md

9d1bf82

Update README.md

ffba179

sync fork with upstream

905c713

fix readme

acc2109

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

d470523

* 'master' of https://github.com/Microsoft/vscode-python: Fixes #56 list all environments (#219) Fixes #57 Disable activation on debugging (#220) Fixes #26 Do not run linters when linters are disabled (#222)

merged upstream

d392e8b

Merge remote-tracking branch 'upstream/master'

92f775f

* upstream/master: Fix typo in README.md (#252) Disable linter without workspaces (#241)

Merge remote-tracking branch 'upstream/master'

32a6e53

* upstream/master: Fix feedback service (#246) Fix django context initializer (#248) disable generation of tags file upon extension load (#264)

Merge remote-tracking branch 'upstream/master'

4b30f2c

* upstream/master: Resolve pythonPath before comparing it to shebang (#273)

Merge remote-tracking branch 'upstream/master'

eff4792

Merge remote-tracking branch 'upstream/master'

4553c28

Merge remote-tracking branch 'upstream/master'

3c6520a

* upstream/master: Fix debugging tests (#304)

Merge remote-tracking branch 'upstream/master'

966e516

* upstream/master: Remove jupyter functionality in favor of Jupyter extension (#302) Drop Python 2 URLs (#307)

Merge remote-tracking branch 'upstream/master'

63d2d65

* upstream/master: Remove setting python.formatting.formatOnSave in favor of the vs code setting (#312)

Merge remote-tracking branch 'upstream/master'

f6d469e

* upstream/master: Remove setting linting.lintOnTextChange as it was never implemented (#315)

Merge remote-tracking branch 'upstream/master'

029e055

* upstream/master: Fix travis build error (#326)

Merge remote-tracking branch 'upstream/master'

e8c71c0

* upstream/master: add new npm deps with improved gulp for dev (#328)

Merge remote-tracking branch 'upstream/master'

51cf9d2

* upstream/master: Update version of inversify package (#329)

Merge remote-tracking branch 'upstream/master'

7aadc43

* upstream/master: Document our dev process (#330)

Merge remote-tracking branch 'upstream/master'

f0f5c59

* upstream/master: Document contribution to the code along with coding standards (#321)

Merge remote-tracking branch 'upstream/master'

b2b9da9

* upstream/master: Add Simplified Chinese translation of commands (#240)

Merge remote-tracking branch 'upstream/master'

30a4091

* upstream/master: Fix package.json (#347)

Merge remote-tracking branch 'upstream/master'

b16d2f9

* upstream/master: #34, #110 - suppress Intellisense in strings and comments (#339) Re-factor code python execution framework (#345)

Merge remote-tracking branch 'upstream/master'

c8db345

* upstream/master: Fix linters to make use of the new python code execution framework (#360) Update the versioning scheme (#356) Make npm happy in regards to line endings (#357)

Merge remote-tracking branch 'upstream/master'

0df7f16

* upstream/master: Ensure python path is not set if already set in user settings (#369) Use 'an' rather than 'a' before vowel words (#373)

Merge remote-tracking branch 'upstream/master'

3ccc881

* upstream/master: Use new environment variable parser (#362)

Merge remote-tracking branch 'upstream/master'

bb0709e

DonJayamanne added 4 commits December 15, 2017 13:11

fix tests

1bdc95d

fix tests

0313000

disable message D102

e7eb19e

fix bug introduced into test

d09b3ef

DonJayamanne changed the title ~~Refactor extension to completely remove old way of spawning python processes~~ WIP - Refactor extension to completely remove old way of spawning python processes Dec 15, 2017

fix tests

0467d4c

DonJayamanne changed the title ~~WIP - Refactor extension to completely remove old way of spawning python processes~~ Refactor extension to completely remove old way of spawning python processes Dec 15, 2017

brettcannon suggested changes Dec 15, 2017

View reviewed changes

fixed code review comments

ffdfd5d

DonJayamanne added 2 commits December 18, 2017 16:46

Merge remote-tracking branch 'upstream/master'

b6b2531

* upstream/master: Refactor formatters to use new execution framework (#426)

merged master

2b21cec

brettcannon approved these changes Dec 19, 2017

View reviewed changes

Fixed code review comments

21601f2

DonJayamanne added 2 commits December 19, 2017 14:14

Merge remote-tracking branch 'upstream/master'

8d8d2fc

* upstream/master: Release 0.9.1 (#458) Add localization for Japanese (#434) Add Svn to uriSchemesToIgnore (#443)

merged master

6641ec8

DonJayamanne changed the title ~~Refactor extension to completely remove old way of spawning python processes~~ Refactor extension to remove old way of spawning python processes Jan 2, 2018

brettcannon suggested changes Jan 2, 2018

View reviewed changes

DonJayamanne added 2 commits January 2, 2018 14:26

fixed code review comments

25d3758

removed unused var

7a45b2d

brettcannon approved these changes Jan 3, 2018

View reviewed changes

DonJayamanne merged commit aec4fb3 into microsoft:master Jan 3, 2018

DonJayamanne deleted the RefactorJediAndOthers branch January 4, 2018 19:49

DonJayamanne added a commit that referenced this pull request Jan 9, 2018

Merge remote-tracking branch 'upstream/master'

c425a55

* upstream/master: Refactor parsing of environment variables (after 439) (#466) Refactor extension to remove old way of spawning python processes (#439)

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

Refactor extension to remove old way of spawning python processes #439

Refactor extension to remove old way of spawning python processes #439

DonJayamanne commented Dec 15, 2017

brettcannon Dec 15, 2017

DonJayamanne Dec 15, 2017

brettcannon Dec 15, 2017

brettcannon Dec 15, 2017

brettcannon Dec 15, 2017

DonJayamanne Dec 15, 2017

DonJayamanne Dec 15, 2017

brettcannon Dec 15, 2017

DonJayamanne commented Dec 15, 2017

DonJayamanne commented Dec 19, 2017

brettcannon left a comment

brettcannon Dec 19, 2017

DonJayamanne Jan 2, 2018

DonJayamanne commented Dec 19, 2017

brettcannon Jan 2, 2018

brettcannon Jan 2, 2018

brettcannon Jan 2, 2018

DonJayamanne Jan 2, 2018

DonJayamanne commented Jan 2, 2018

Refactor extension to remove old way of spawning python processes #439

Refactor extension to remove old way of spawning python processes #439

Conversation

DonJayamanne commented Dec 15, 2017

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 commented Dec 15, 2017

DonJayamanne commented Dec 19, 2017

brettcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne commented Dec 19, 2017

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 commented Jan 2, 2018