Skip to content

Auto activation of environment in new terminals #2277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Sep 7, 2018

Conversation

DonJayamanne
Copy link

@DonJayamanne DonJayamanne commented Jul 30, 2018

Fixes #1387

  • Title summarizes what is changing
  • Includes a news entry file (remember to thank yourself!)
  • Unit tests & code coverage are not adversely affected (within reason)
  • Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
  • Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)
  • [n/a] Dependencies are pinned (e.g. "1.2.3", not "^1.2.3")
  • [n/a] package-lock.json has been regenerated if dependencies have changed

@DonJayamanne DonJayamanne changed the title Auto activation of environment in new terminals WIP - Auto activation of environment in new terminals Jul 30, 2018
@brettcannon brettcannon changed the title WIP - Auto activation of environment in new terminals [WIP] Auto activation of environment in new terminals Aug 1, 2018
@DonJayamanne DonJayamanne changed the title [WIP] Auto activation of environment in new terminals Auto activation of environment in new terminals Sep 7, 2018
@DonJayamanne DonJayamanne requested a review from d3r3kk September 7, 2018 19:02
Copy link

@d3r3kk d3r3kk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that we are switching some of our test suites from system to unit tests! Let's do more of that when we can...

That being said, is there anything we can do, any event available we can hook into, to get rid of that sleep?

I've approved anyways because I realize this is a 'pre-existing condition', but if there is a way to get proper notification of when things are ready I'd love to get that made into an issue...


// Give the command some time to complete.
// Its been observed that sending commands too early will strip some text off in VS Terminal.
const delay = (terminalShellType === TerminalShellType.powershell || TerminalShellType.powershellCore) ? 1000 : 500;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really love to see if we can get an event from the shell when it is 'ready' instead of this sleep stuff. Is there any issue related to this that VSCode knows about/can do anything about?

Do we know why this is the case at all?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we can get an event from the shell when it is 'ready' instead o

Not available.

Do we know why this is the case at all?

Powershell is slow. VS Code doesn't get any feedback when process in terminal has completed. Basically the stream is ready to write anytime.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we just use the VS Code api (which in turn doesn't provide any feedback on when stuff is ready or not, explained above).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, is there anything we can do, any event available we can hook into, to get rid of that sleep?

This has been an ongoing issue in VSC. Unfortunately nothing we can do at this stage.

suite(testSuiteName, () => {
suite(testSuiteName, async function () {
// tslint:disable-next-line:no-invalid-this
this.timeout(5000); // Activation of terminals take some time (there's a delay in the code to account for VSC Terminal issues).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BOOOOOOO

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I know you have to up the limit here a bit in order to cope, but still, boo.

@DonJayamanne DonJayamanne merged commit 4019b98 into microsoft:master Sep 7, 2018
@DonJayamanne DonJayamanne deleted the autoActivate branch October 2, 2018 22:46
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically activate terminals created by the user when not using the Python command
2 participants