Activate environment in terminal #614

DonJayamanne · 2018-01-23T01:14:03Z

Fixes #383

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)

* upstream/master: update reflect metadata dependency (#604)

codecov · 2018-01-23T01:56:06Z

Codecov Report

Merging #614 into master will increase coverage by 2.6%.
The diff coverage is 90.25%.

@@            Coverage Diff            @@
##           master     #614     +/-   ##
=========================================
+ Coverage   58.06%   60.67%   +2.6%     
=========================================
  Files         225      228      +3     
  Lines       10152    10125     -27     
  Branches     1751     1753      +2     
=========================================
+ Hits         5895     6143    +248     
+ Misses       4252     3977    -275     
  Partials        5        5

Impacted Files	Coverage Δ
src/client/common/application/types.ts	`100% <ø> (ø)`	⬆️
src/client/terminals/types.ts	`100% <ø> (ø)`	⬆️
src/client/interpreter/locators/services/conda.ts	`100% <ø> (ø)`	⬆️
src/client/common/platform/types.ts	`100% <ø> (ø)`	⬆️
...erminals/codeExecution/djangoShellCodeExecution.ts	`100% <100%> (ø)`	⬆️
src/client/interpreter/display/index.ts	`97.43% <100%> (-0.44%)`	⬇️
src/client/extension.ts	`93.95% <100%> (-0.09%)`	⬇️
src/client/common/terminal/types.ts	`100% <100%> (ø)`	⬆️
src/client/terminals/serviceRegistry.ts	`100% <100%> (ø)`	⬆️
src/client/providers/replProvider.ts	`100% <100%> (+40%)`	⬆️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba069d2...8bf8a17. Read the comment docs.

DonJayamanne · 2018-01-23T06:06:20Z

@brettcannon @MikhailArkhipov
I'm finishing up some minor issues and tests.
Please could you review this.

MikhailArkhipov · 2018-01-23T20:34:21Z

src/client/common/platform/fileSystem.ts

 import { IFileSystem, IPlatformService } from './types';

 @injectable()
 export class FileSystem implements IFileSystem {
-    constructor( @inject(IServiceContainer) private platformService: IPlatformService) { }
+    constructor( @inject(IPlatformService) private platformService: IPlatformService) { }


Up to you, there is always a discussion if one should pass container or just minimal service(s). However, if we are to switch to our own container implementation, it may not be injectable so we'll have to pass around container anyway.

I agree, just didn't want to change existing code.
The plan going forward is to only pass the service container

MikhailArkhipov · 2018-01-23T20:41:20Z

src/client/common/terminal/environmentActivationProviders/commandPrompt.ts

+    }
+    public async getActivationCommands(interpreter: PythonInterpreter, targetShell: TerminalShellType): Promise<string | string[] | undefined> {
+        // Dependending on the target shell, look for the preferred script file.
+        const scriptsInOrderOfPreference = targetShell === TerminalShellType.commandPrompt ? ['activate.bat', 'activate.ps1'] : ['activate.ps1', 'activate.bat'];


activate.cmd ?

I couldn't find such a file in the python virtual environment directories.

OK. bat is usually old DOS thing, XP, win7 used to use cmd. Poweshell comes with Win10 preinstalled I think.

MikhailArkhipov · 2018-01-23T20:43:55Z

src/client/common/terminal/environmentActivationProviders/commandPrompt.ts

+            return;
+        }
+
+        const quotedScriptFile = scriptFile.indexOf(' ') > 0 ? `"${scriptFile}"` : scriptFile;


Looks like sequence used several times, perhaps move to findScriptFile ?

Don't think thats right. The find shoudl only return the file, this is responsible for building the command for execution, and when a file has a space, then we need to quote it.
Can create a separate helper such as "quoteFilePath"

MikhailArkhipov · 2018-01-23T20:44:35Z

src/client/common/terminal/environmentActivationProviders/conda.ts

+// import { IServiceContainer } from '../../../ioc/types';
+// import { TerminalShellType } from '../types';
+// import { BaseActivationCommandProvider } from './baseActivationProvider';
+


tslint hates all-comments

supposed to be removed.

MikhailArkhipov · 2018-01-23T20:59:39Z

src/client/common/terminal/helper.ts

+        const supportedProviders = providers.filter(provider => provider.isShellSupported(terminalShellType));
+
+        for (const provider of supportedProviders) {
+            const activationCommands = await provider.getActivationCommands(interperterInfo, terminalShellType);


So we pick just the first one, right?

MikhailArkhipov · 2018-01-23T21:06:32Z

src/client/common/terminal/factory.ts

+        return this.terminalServices.get(id)!;
+    }
+    private getTerminalId(resource?: Uri, title?: string): string {
+        const terminalTitle = title ? title! : '';


Bang probably not needed

Perhaps something generic title, like 'Python'?

MikhailArkhipov · 2018-01-23T21:11:34Z

src/client/common/terminal/types.ts

+
+export interface ITerminalActivationCommandProvider {
+    isShellSupported(targetShell: TerminalShellType): boolean;
+    getActivationCommands(interpreter: PythonInterpreter, targetShell: TerminalShellType): Promise<string | string[] | undefined>;


Might be simpler to have it always return an array, fewer cases to handle for the caller

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

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 January 22, 2018 17:06

revert change

7d82c77

Merge remote-tracking branch 'upstream/master'

685b683

* upstream/master: update reflect metadata dependency (#604)

merged master

c165901

fix detection of conda tests

fb6142a

DonJayamanne added 3 commits January 22, 2018 18:15

tests for bash,cshell and fish activation scripts

0ff33e9

better test suite names

7ba90db

added more tests

faf48b5

DonJayamanne added 3 commits January 23, 2018 08:45

added tests for env activation

605fabe

added tests for activation commands

96a4571

fix linter errors

a8b4e7c

MikhailArkhipov reviewed Jan 23, 2018

View reviewed changes

unit tests for repl provider

001fc30

MikhailArkhipov reviewed Jan 23, 2018

View reviewed changes

MikhailArkhipov approved these changes Jan 23, 2018

View reviewed changes

DonJayamanne added 5 commits January 23, 2018 13:24

fix code review comments

b9d503b

extension method to quote strings for cmd args

264e52a

give preference to scripts based on shell

3e7c1aa

fix types of script files supported by shells

2985dad

activate terminal before displaying it

8bf8a17

DonJayamanne merged commit 0253995 into microsoft:master Jan 23, 2018

DonJayamanne changed the title ~~WIP - Activate environment in terminal~~ Activate environment in terminal Jan 23, 2018

DonJayamanne deleted the activateEnvInTerminal branch January 25, 2018 19:33

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

Activate environment in terminal #614

Activate environment in terminal #614

DonJayamanne commented Jan 23, 2018

codecov bot commented Jan 23, 2018 •

edited

Loading

DonJayamanne commented Jan 23, 2018

MikhailArkhipov Jan 23, 2018

DonJayamanne Jan 23, 2018

MikhailArkhipov Jan 23, 2018

DonJayamanne Jan 23, 2018

MikhailArkhipov Jan 23, 2018

MikhailArkhipov Jan 23, 2018

DonJayamanne Jan 23, 2018

MikhailArkhipov Jan 23, 2018

DonJayamanne Jan 23, 2018

DonJayamanne Jan 23, 2018

MikhailArkhipov Jan 23, 2018

DonJayamanne Jan 23, 2018

MikhailArkhipov Jan 23, 2018

MikhailArkhipov Jan 23, 2018

DonJayamanne Jan 23, 2018

MikhailArkhipov Jan 23, 2018

DonJayamanne Jan 23, 2018

Activate environment in terminal #614

Activate environment in terminal #614

Conversation

DonJayamanne commented Jan 23, 2018

codecov bot commented Jan 23, 2018 • edited Loading

Codecov Report

DonJayamanne commented Jan 23, 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

codecov bot commented Jan 23, 2018 •

edited

Loading