Loading Pull Request List via GraphQL - #1022
Loading Pull Request List via GraphQL#1022Stanley Goldman (StanleyGoldman) wants to merge 11 commits into
Conversation
# Conflicts: # src/github/githubRepository.ts
Stanley Goldman (StanleyGoldman)
left a comment
Stanley Goldman (StanleyGoldman)
left a comment
There was a problem hiding this comment.
Some old artifacts were found and removed.
| interface PageInformation { | ||
| pullRequestPage: number; | ||
| hasMorePages: boolean; | ||
| } |
There was a problem hiding this comment.
This seems to be an artifact from an old implementation.
| public collapsibleState: vscode.TreeItemCollapsibleState; | ||
| public prs: PullRequestModel[]; | ||
| public fetchNextPage: boolean = false; | ||
| public repositoryPageInformation: Map<string, PageInformation> = new Map<string, PageInformation>(); |
There was a problem hiding this comment.
As does this.
cfa7d69 to
fd8a023
Compare
| state: pullRequestItem.state, | ||
| title: pullRequestItem.title, | ||
| user: pullRequestItem.author | ||
| }; |
There was a problem hiding this comment.
Mapping the graphql object to an object the PullRequestModel consumes. Not thrilled about doing this here.
There was a problem hiding this comment.
the file src/github/utils.ts has functions for mapping response types to normalized types
There was a problem hiding this comment.
Thanks, i'll move the code.
Tilde Thurium (annthurium)
left a comment
Tilde Thurium (annthurium)
left a comment
There was a problem hiding this comment.
Looks good! it's interesting to see what non-Relay gql code looks like since I've only ever used Relay.
Left a few comments -- would be good for somebody else to be the approving reviewer though since I am just getting familiar with this code base.
| } | ||
| async getPullRequestsGraphQL(type: PRType, nextCursor?: string|null):Promise<PullRequestListResponse|undefined> { | ||
| const { remote, query, octokit } = await this.ensure(); | ||
| const currentUser = octokit && (octokit as any).currentUser; |
There was a problem hiding this comment.
just out of curiosity, why octokit as any? Do we not have ts bindings for octokit or something?
There was a problem hiding this comment.
I have a similar question as I'm pretty sure the bindings exist.
There was a problem hiding this comment.
Yeah, we do have bindings for octokit, but currentUser is something we've added ourselves
We also added an octokit.ts file a while ago that modifies the typings slightly, there was something that should have been nullable that wasn't. currentUser can be added there to get rid of our any casts. But I think we should ultimately try to get rid of that file by correcting the typings upstream
| } | ||
| ) | ||
| .filter(item => item !== null) as PullRequestModel[]; | ||
| let filter = `type:pr is:open repo:${remote.owner}/${remote.repositoryName}`; |
There was a problem hiding this comment.
Might be a little edge casey, but do we need to guard against the remote being null here?
There was a problem hiding this comment.
I would say yes, but looking at all the other calls to ensure to get remote seem to make the assumption that remote will not be null.
| const currentUser = octokit && (octokit as any).currentUser; | ||
| const currentUserLogin: string = currentUser.login; | ||
|
|
||
| private async getAllPullRequests(page?: number): Promise<PullRequestData | undefined> { |
There was a problem hiding this comment.
I don't think we should remove this - we still need to support GHE that doesn't have GraphQL
| } else { | ||
| throw e; | ||
| } | ||
| if(!!nextCursor) { |
There was a problem hiding this comment.
Just curious why you use if(!!condition) here, instead of if(condition)
|
Thanks for the PR Stanley Goldman (@StanleyGoldman). Since the goal of this PR was to enable #681, I'm going to close it. We already have #681 as of the last stable release. |
6 participants
In order to implement (#1020), this pull request changes functionality to query for the pull request list via GraphQL. REST API calls to obtain Pull Request list items do not include the
mergeableflag.