Conversation
|
@axosoft-ramint In the last commit I'm grouping detected PR-identity by providers, because thought that if we know that an URL matches to a GitLab then no need to search for it in GitHub. But if it looks too compicated, then then last commit can be rejected. |
1c0746a to
0feb173
Compare
ramin-t
left a comment
There was a problem hiding this comment.
As it stands, this doesn't quite feel ready. The utils and test files that are provider-specific should be organized into provider-specific directories. The url identity being a mapped object by provider id feels confusing - the identity itself should just come in on the function. Some of the changes to the PR utils need either explanation or rewrite to make more sense (why are we defining an undefined const and then returning it without touching it?). The same function being defined in the PR utils but then also implemented specifically for integrations doesn't quite make sense to me. The checkoutable logic in launchpad shouldn't be necessary - we should always have at least a defined head ref and remote url for a PR, or else we're not fetching enough info.
Can talk on this further on your return.
0feb173 to
e58350a
Compare
e58350a to
8f03c1c
Compare
4ae8ec0 to
89e0292
Compare
89e0292 to
89bd972
Compare
|
@axosoft-ramint
fixed
fixed
fixed
I access the necessary implementation of the function through the integration instance that's why it's in method. But I'm not sure about one thing: vscode-gitlens/src/plus/launchpad/launchpadProvider.ts Lines 584 to 611 in 89bd972
Fixed. |
src/git/models/pullRequest.ts
Outdated
| return { | ||
| remote: { | ||
| url: pr.refs?.head?.url, | ||
| url: pr.repository.url, |
There was a problem hiding this comment.
This changes the url ref to the base repository. This would break the model for PRs where the PR is targeting a different repository and (like when the head ref is from someone's fork).
Commands that use this like "open the PR's repository" or "open in worktree" will break in those cases.
There was a problem hiding this comment.
@axosoft-ramint
Thank you for catching it.
Question here. pr.refs?.head?.url is a link to a branch to to the repo. It fails matching to uniqueRemoteUrls in the LaunchpadProvider when I try to swtichTo the branch.
So I see 3 ways to fix it:
- Add
pr.refs.head.repoUrlon the specific provider level. - Fix the matching to
uniqueRemoteUrlsin launchpadProvider - Cut it right here:
url: someOperationOnRefUrl(pr.refs?.head?.url)
I think that the first option is the best because it can respect specifics of each type of integration. What do you think?
There was a problem hiding this comment.
@sergeibbb pr.hefs?.head?.url should be the link to the remote (repository) that the head ref is hosted on, not a link to a branch - at least, with GitHub this is the case. Are you finding that with GitLab, it is giving a path to a branch? If so, whatever we are using to convert GitLab PRs into our PR model, I think we are plugging the wrong datapoint into that property and should start there.
There was a problem hiding this comment.
@axosoft-ramint
GitLab REST API does not provide necessary information about source and target repositories. So, I've fixed it by switching to GraphQL. (ffce252)
However I haven't found a good way to search MRs via GraphQL. So I've solved it with 2 requests. I still use REST to search and then I use PRs' IDs to retrieve the rest of the fields from GraphQL. (107da17)
cc @nzaytsev
| const prUrlIdentity: PullRequestUrlIdentity | undefined = await this.getPullRequestIdentityFromMaybeUrl(search); | ||
| const result: { readonly value: SearchedPullRequest[]; duration: number } = { | ||
| value: [], | ||
| duration: 0, | ||
| }; | ||
|
|
||
| const connectedIntegrations = await this.getConnectedIntegrations(); |
There was a problem hiding this comment.
If we're already getting the connected integrations below, maybe we can just pass them into this.getPullRequestIdentityFromMaybeUrl
| ): Promise<undefined | TimedResult<SearchedPullRequest[] | undefined>> => { | ||
| const prs = await withDurationAndSlowEventOnTimeout( | ||
| integration?.searchPullRequests(search, undefined, cancellation), | ||
| integration?.searchPullRequests(search, undefined, cancellation), // |
There was a problem hiding this comment.
Empty comment line at the end
ramin-t
left a comment
There was a problem hiding this comment.
On testing, the only issue I'm seeing is that owner/repo are not showing up on GitLab search results:
They do for GitLab PRs that are not search results.
Apart from that, some minor nitpicks that should be easy to change:
| if (!connected) return undefined; | ||
|
|
||
| const pr = this.container.cache.getPullRequest(id, resource, this, () => ({ | ||
| const pr = await this.container.cache.getPullRequest(id, resource, this, () => ({ |
There was a problem hiding this comment.
This is unnecessary since we return the promise in the next line, similar to other cases. If anything, I would suggest instead that we just combine the two lines to return this.container.cache.get... and do the same in other places where this pattern occurs across the file.
There was a problem hiding this comment.
@axosoft-ramint
I did it because I set a breakpoint at this place it was convenient to see a resolved promise.
src/plus/integrations/integration.ts
Outdated
| cancellation?: CancellationToken, | ||
| ): Promise<PullRequest[] | undefined>; | ||
|
|
||
| getPullRequestIdentityFromMaybeUrl?(search: string): PullRequestUrlIdentity | undefined; |
There was a problem hiding this comment.
Let's follow a similar pattern for generic and provider-specific function implementation and naming here as we do for other functions, in case we want to expand on the base logic later:
- Have a
protected getProviderPullRequestIdentityFromMaybeUrlthat can be implemented by specific providers (i.e. GitHub, GitLab, etc.) - Have a
getPullRequestIdentityFromMaybeUrlthat, for now, just doesreturn getProviderPullRequestIdentityFromMaybeUrland later if we want, we can layer in caching there.
| ); | ||
| } | ||
|
|
||
| async getPullRequestIdentityFromMaybeUrl( |
There was a problem hiding this comment.
I suggest calling this one getPullRequestIdentityFromSearch to avoid making it seem as if line 607 is a recursive call to the same function.
|
@axosoft-ramint
fixed |
#3788
Description
Checklist
Fixes $XXX -orCloses #XXX -prefix to auto-close the issue that your PR addresses