Proposal TerminalExitStatus.reason - #152833
Conversation
| * get cut off. If immediate kill any terminal processes immediately. | ||
| * @param isShutdown Whether the kill was triggered by lifecycle shutdown | ||
| */ | ||
| dispose(immediate?: boolean): void; |
There was a problem hiding this comment.
Is immediate deprecated? There was only one place that used it (in terminalQuickAccess.ts) but I changed it to terminalService.safeDisposeTerminal so it's the same as other calls.
If not I'll revet this change.
There was a problem hiding this comment.
I'm not totally sure what the current state of this is, but it was always a bit of a hack to add a param to dispose like this so I'd want to move away from it if we were to change things in this area.
|
Daniel Imms (@Tyriar) Does proposed change make sense to you? We would really appreciate such API like stable though to unblock our customers. 🙏 |
Daniel Imms (Tyriar)
left a comment
There was a problem hiding this comment.
The idea seems reasonable, I'll make a note to bring this to the next API sync which will be in 2 weeks.
| * get cut off. If immediate kill any terminal processes immediately. | ||
| * @param isShutdown Whether the kill was triggered by lifecycle shutdown | ||
| */ | ||
| dispose(immediate?: boolean): void; |
There was a problem hiding this comment.
I'm not totally sure what the current state of this is, but it was always a bit of a hack to add a param to dispose like this so I'd want to move away from it if we were to change things in this area.
c34b852 to
4e102e6
Compare
|
Daniel Imms (@Tyriar) thank you! Is API sync internal or we can follow it somehow as well? |
|
Anton Kosyakov (@akosyakov) it is internal and we will post a summary after that happens |
|
Discussed at the API sync and we think the shape looks good. Just the one question above |
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
|
I forgot to ask this, how long does it take for a proposal API to become stable? I just realized that even if this is merged we won't be able to use it in our extension and publish it to the marketplace 😅 |
|
It needs to stay proposed for at least a month, this one's not complex so 1 month seems right |
Daniel Imms (@Tyriar) Megan Rogge (@meganrogge) Thank you a lot 🙏 Is it good to land? I believe Jean Pierre (@jeanp413) addressed the feedback. |
Daniel Imms (Tyriar)
left a comment
There was a problem hiding this comment.
Looks good, will merge after a full test pass
fcbef68
This PR fixes #130231
Implements API proposal
TerminalExitStatus.reasonto address issue #130231Added
Shutdown(main motivation for #130231) andUnknown(i.e. any other reasons) reasonsSide note, after working on this, I would say that firing
vscode.window.onDidCloseTerminalon shutdown is more of a bug than a feature, if some extension terminal logic is required to execute on shutdown then usingcontext.subscriptionswould be preferred I think but changingonDidCloseTerminalbehavior would be a breaking change 🤔cc Daniel Imms (@Tyriar)