Add a setting to activate environment in the current open terminal #7665
Conversation
|
|
popzxc
force-pushed
the
activate-current-terminal
branch
from
8938996
to
42af186
Compare
popzxc
force-pushed
the
activate-current-terminal
branch
from
42af186
to
7aa59c1
Compare
|
@DonJayamanne can this PR be reviewed? |
|
@microsoft/pvsc-team /cc |
|
The original issue needs spec, so we'll have to wait from approval from the PMs before we can merge this. |
|
@luabud can do the review / final approval for this, she is out this week if we don't mind waiting until next week to review the setting |
|
We might need to add some code to track the usage (telemetry) |
|
@luabud I feel really awkward for reminding about myself again, but can I please get some feedback? |
|
@popzxc I'm SO sorry for the delay! I got caught up with other tasks and missed this one, my bad 😞 But thank you so much for working on this! It's working great 🎉. The setting name seems good, I think "python.terminal.activateEnvInCurrentTerminal" describes the intent well. As Don mentioned, it'd be nice if we can add telemetry to track usage - this helps us prioritize maintenance and feature work, and sometimes with error/bug detection 😊. If I understood it correctly, the terminal is only activated when the extension is activated (i.e. on first launch), is that correct? Although this seems to be the safest approach to take, at least for me the expected behavior would be to activate it every time the interpreter changes (assuming users would only change enable the activateEnvInCurrentTerminal setting if they are aware of its behavior). I believe the worst-case scenario would be if someone is half-way through a command or with a REPL open in the terminal - which would throw errors and the terminal wouldn't be activated, but this is an issue similar to what we already have when e.g. sending selections to the same terminal after the user quits the REPL. What do you all think? |
|
Well, the suggested approach seems completely reasonable to me (I don't really often change the interpreter within workspace, so I just forgot to think about it), I will implement that soon. Regarding the telemetry question: I think it would be the best if you will guide me which kind of information should be sent, since I don't feel qualified enough to decide on my own. As I can see it, I can add a variant "activatecurrentterminalsetting" into Will that be enough or should it be something else? |
|
@popzxc this PR brought up a long discussion in the team 😁. First, I created a separate issue for the telemetry work, so that you don't have to worry about it 😊. Secondly, there were some concerns regarding the approach I suggested as well as the current behavior with this PR, and we all agreed we need to discuss it further. We opened an issue so we can investigate the feasibility of a few other options and we'll get back to you once we have a decision (mostly likely in a week or two). And again, thank you so much for working on this 👍. |
|
Sure, no problem :) |
|
Thank you so much for your patience @popzxc! We finished the investigation and we're deciding to go with your approach 😊. |
| @@ -14,6 +15,15 @@ export class TerminalProvider implements Disposable { | |||
| constructor(private serviceContainer: IServiceContainer) { | |||
| this.registerCommands(); | |||
| } | |||
| public async initialize(currentTerminal: Terminal | undefined) { | |||
| const configuration = this.serviceContainer.get<IConfigurationService>(IConfigurationService); | |||
| const pythonSettings = configuration.getSettings(); | |||
There was a problem hiding this comment.
Shoot. Actually the setting added is not a user setting, but a workspace folder setting😅
So while fetching the settings, one have to provide the workspace folder(or the active resource)
Should be configuration.getSettings(this.getActiveResource()) instead. Will do it as part of #8004
For #5330
Since a generic solution for that issue is quite hard to imagine, I decided to make this functionality optional.
Thus, people who really wants to get environment activated in the current open terminal, can enable this behavior in settings, and everyone else won't be affected.
Test plan is updated as appropriatepackage-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)The wiki is updated with any design decisions/details.