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

Consider making handleTerminalLink return a Promise #101391

Closed
alexr00 opened this issue Jun 30, 2020 · 3 comments
Closed

Consider making handleTerminalLink return a Promise #101391

alexr00 opened this issue Jun 30, 2020 · 3 comments
Assignees
Labels
api bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders terminal Integrated terminal issues terminal-links verified Verification succeeded
Milestone

Comments

@alexr00
Copy link
Member

alexr00 commented Jun 30, 2020

Testing #101300

I was testing with the existing (albeit turned off) document link provider for issues in the GHPRI extension. I wanted to re-use as much code from the document link provider as possible. With the document link provider, resolveDocumentLink can return a promise. To keep a similar pattern I would also expect handleTerminalLink, so I can do any further information resolving there.

@Tyriar
Copy link
Member

Tyriar commented Jun 30, 2020

What would it return? I removed the old TerminalLink.target? property which could be filled in as I thought this was simpler.

@Tyriar Tyriar added the info-needed Issue requires more information from poster label Jun 30, 2020
@eamodio
Copy link
Contributor

eamodio commented Jun 30, 2020

While, I'm not sure handleTerminalLink should return a promise, but I am a little worried about extensions not doing more intensive processing in provideTerminalLinks and instead delaying until handleTerminalLink, which as long as they provide some interaction that is probably fine, but if they don't it could make it seem that vscode is broken -- because I click on a link and nothing happens. Maybe it should return a boolean | Promise<bool> in which case we could show some interaction (or maybe just remove the link decoration from that range)?

@alexr00
Copy link
Member Author

alexr00 commented Jul 1, 2020

I would expect it to return Promise<void>. My testing was focused around sharing code between a document link provider and a terminal link provider, and I had made resolveDocumentLink async, which meant that I couldn't reuse some of that code in handleTerminalLink.

Regarding having more work done in handleTerminalLink: we already expect extensions to do that in resolveDocumentLink. We have already accepted that there could be work happening when someone clicks a link.

@Tyriar Tyriar added api terminal Integrated terminal issues terminal-links under-discussion Issue is under discussion for relevance, priority, approach and removed info-needed Issue requires more information from poster labels Jul 1, 2020
@Tyriar Tyriar added this to the July 2020 milestone Jul 1, 2020
@Tyriar Tyriar closed this as completed in cb31fc6 Jul 15, 2020
@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug and removed under-discussion Issue is under discussion for relevance, priority, approach labels Aug 3, 2020
@alexr00 alexr00 added the verified Verification succeeded label Aug 5, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders terminal Integrated terminal issues terminal-links verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants
@eamodio @Tyriar @alexr00 and others