Conversation
alexr00
left a comment
There was a problem hiding this comment.
This is something other users have actually asked for too, so it's cool that we'll have it. What are planning to use if for?
…st-github into osortega/repository-description-api
alexr00
left a comment
There was a problem hiding this comment.
I still don't think this is the right approach to give chat info about the active PR. This is a bad pattern:
- PR extension exposes active PR API
- Copilot extension calls PR extension's API
- Copilot extension adds this context to chat.
This means that every time we have an extension that wants to contribut context, the Copilot extension needs to know about it.
Instead, we should have VS Code extension API:
export interface ChatContextProvider {
// Returns a short string context. Shortness will be enforced.
provideContext(): Promise<string>
}
registerChatContextProvider(provider: ChatContextProvider);
| } | ||
|
|
||
| async getRepositoryDescription(uri: vscode.Uri) { | ||
| const repo = this.getGitProvider(uri)?.repositories.find(r => r.rootUri.fsPath === uri.fsPath); |
There was a problem hiding this comment.
Instead of checking for equality with the rootUri, we should find the git repo where the uri is a subpath of the git repo's uri. This could be multiple repos.
There was a problem hiding this comment.
We have a method that does this already for folder managers, I think you can just use that.
vscode-pull-request-github/src/github/repositoriesManager.ts
Lines 131 to 132 in 3731024
There was a problem hiding this comment.
Ahhh thanks for this pointer! Makes it so much easier
|
|
||
| if (folderManagerForRepo && folderManagerForRepo.gitHubRepositories.length > 0) { | ||
| const repositoryMetadata = await folderManagerForRepo.gitHubRepositories[0].getMetadata(); | ||
| const pullRequest = folderManagerForPR?.activePullRequest ?? PullRequestOverviewPanel.currentPanel?.getCurrentItem(); |
There was a problem hiding this comment.
Let's keep this just to the currently checked out PR, not the PR in the webview. When users have asked for this feature in the past they really just want to know about the currently checked out PR.
There was a problem hiding this comment.
Good idea, removed the logic that gets it through the panel
alexr00
left a comment
There was a problem hiding this comment.
Just to emphasize that the Copilot extension should not know about the PR extension: I want to delete the PR title and description stuff from the Copilot extension and move it into the PR extension and will do so in a future debt week.
alexr00
left a comment
There was a problem hiding this comment.
I'm about to make a pre-release, and I know you've been waiting for this to get in. I pushed some very minor chnages in the interest of getting this into the pre-release sooner. Thank you for addressing my comments!
Adding an API for fetching repository description