Skip to content
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

Allow terminal environment to be set when created via extension API #20446

Closed
Tyriar opened this issue Feb 10, 2017 · 19 comments · Fixed by #30352
Closed

Allow terminal environment to be set when created via extension API #20446

Tyriar opened this issue Feb 10, 2017 · 19 comments · Fixed by #30352
Assignees
Labels
api feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities terminal Integrated terminal issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Feb 10, 2017

No description provided.

@Tyriar Tyriar added api feature-request Request for new features or functionality terminal Integrated terminal issues labels Feb 10, 2017
@Tyriar Tyriar added this to the Backlog milestone Feb 10, 2017
@Tyriar Tyriar self-assigned this Feb 10, 2017
@Tyriar
Copy link
Member Author

Tyriar commented Feb 10, 2017

/cc @dbaeumer we'll need this for tasks in an extension
/cc @daviwil

@daviwil
Copy link
Contributor

daviwil commented Feb 10, 2017

Sweet, thanks!

@Tyriar Tyriar added the help wanted Issues identified as good community contribution opportunities label May 28, 2017
@Tyriar
Copy link
Member Author

Tyriar commented May 28, 2017

Opening to PRs, the ideal change would plum through a new option in the terminal API "env" which includes additions to the environment. You can look a search on "waitOnExit" to see most of the places that need to change https://github.com/Microsoft/vscode/search?l=TypeScript&q=waitOnExit&type=&utf8=%E2%9C%93

@dbaeumer
Copy link
Member

@Tyriar we might to think about this in the light of tasks being API now. If a extension other could execute a task programmatically then all is there. A task can already specify its env.

@Tyriar
Copy link
Member Author

Tyriar commented May 30, 2017

@dbaeumer I know that [platformio]](http://platformio.org/) needs this, going through tasks is more restrictive that the terminal so they probably wouldn't want to do that.

@ivankravets
Copy link

Hi all,

Ivan from @platformio. Thanks a lot for your work!

  1. A task can already specify its env.

That is the first with which I stuck. We can't define custom environment variables in tasks.json per each task or as common for all. Again, we use some hook for that.

  1. A custom environment for built-in Terminal is very important. We use dirty hook now. vscode.window.createTerminal('PlatformIO', env=dict) would be good!

@Tyriar
Copy link
Member Author

Tyriar commented May 30, 2017

@dbaeumer do you have a link to the new task contribution extension API?

@dbaeumer
Copy link
Member

Was in vscode.proposed.d.ts in 1.12 and is now in vscode.d.ts. See https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts#L3766

It still might change until we ship 1.1.3

@Tyriar
Copy link
Member Author

Tyriar commented May 31, 2017

I see we're deferring it. @ivankravets you might find this new API useful in an upcoming version e98c09c

@ivankravets
Copy link

ivankravets commented May 31, 2017

Is possible to subscribe to onDidTaskRun()? Where would be an ability to catch up some tasks and perform some operations. We have pre and post callbacks in Atom Build for each task.

Simple use case. User open Serial Monitor via integrated Terminal, we see it. He triggers "Upload" task, we temporary close opened Serial Port Monitors/Terminals, then uploading firmware. If a task fails with an error, we don't re-open Monitor. However, if the upload was successful, we re-open last Serial Port Monitor.

Thanks for the hints!

@dbaeumer
Copy link
Member

dbaeumer commented Jun 1, 2017

@ivankravets currently not but this is definitely something we can provide. Could you open an issue please.

@ivankravets
Copy link

@dbaeumer Thanks, done! #29855

@Tyriar
Copy link
Member Author

Tyriar commented Jul 10, 2017

@dbaeumer @ivankravets @dbaeumer hi all, @ramya-rao-a put together a PR for this #30352. We were thinking that the new env option in the API should add to the current environment rather than replace it. Wanted to get some opinions here because adding to the environment would certainly be more convenient for the vast majority of cases but we're just worried that people may need to remove keys from the environment. Does adding to the current environment cover your use cases?

@dbaeumer
Copy link
Member

I currently merge / override the environments. This doesn't let you remove an value. We could say if a value is undefined it means to remove.

@ivankravets
Copy link

ivankravets commented Jul 11, 2017

My proposition:

  1. options.env accepts dictionary (key/value object)
  2. Terminal iterates over these keys and REPLACE all occurences in the current process.env which later will be used by terminal instance
  3. I like this @dbaeumer "We could say if a value is undefined it means to remove."

P.S: We should be able not only to override some environemnt variable but also to remove it (set to null/undefined).

Thanks!

@Tyriar
Copy link
Member Author

Tyriar commented Jul 11, 2017

@dbaeumer @ivankravets I like it 😄 @ramya-rao-a let's go additive but if the key exists and it's falsy, remove the key from the environment (note this in the API jsdoc).

@Tyriar Tyriar added the verification-needed Verification of issue is requested label Oct 30, 2017
@roblourens roblourens added the verified Verification succeeded label Nov 1, 2017
@ivankravets
Copy link

How to use it with 1.18.0?

vscode.window.createTerminal('PlatformIO', constants.IS_WINDOWS ? 'cmd.exe' : null, [], { TEST: 13 });

Does not work.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Nov 10, 2017

@ivankravets

code.window.createTerminal({name: 'PlatformIO', shellPath: constants.IS_WINDOWS ? 'cmd.exe' : null, shellArgs: [], env: { TEST: 13 }}

@ivankravets
Copy link

@ramya-rao-a Thanks, it works now!

@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities terminal Integrated terminal issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants