Conversation
loichuder
left a comment
loichuder
left a comment
There was a problem hiding this comment.
Honestly, looks ok to me.
There are probably some typing issues that will arise here and there when doing the actual PR.
It might also be worthwhile splitting the PR in several since this fixes several issues at once.
| mergeRequests: { | ||
| logFile: string; | ||
| log: boolean; | ||
| replayMergedRequests?: boolean; |
There was a problem hiding this comment.
| replayMergedRequests?: boolean; | |
| replayMergedRequests: boolean; |
Might be more explicit to keep it a required setting
| const aMerged = a.merged_at ? new Date(a.merged_at).getTime() : 0; | ||
| const bMerged = b.merged_at ? new Date(b.merged_at).getTime() : 0; |
There was a problem hiding this comment.
| const aMerged = a.merged_at ? new Date(a.merged_at).getTime() : 0; | |
| const bMerged = b.merged_at ? new Date(b.merged_at).getTime() : 0; | |
| const aMergeDate = a.merged_at ? new Date(a.merged_at).getTime() : 0; | |
| const bMergeDate = b.merged_at ? new Date(b.merged_at).getTime() : 0; |
| // Sort merge requests in ascending order of their number (by iid), unless | ||
| // we are replaying merged requests in chronological merge order. |
There was a problem hiding this comment.
| // Sort merge requests in ascending order of their number (by iid), unless | |
| // we are replaying merged requests in chronological merge order. | |
| // Sort merge requests in chronological merge order instead of iid if replaying them |
| mergeRequestIid: number | ||
| ): Promise<GitLabMergeRequest | null> { | ||
| try { | ||
| return await this.gitlabApi.MergeRequests.show( |
There was a problem hiding this comment.
IIRC, you don't have to call await if you return a Promise.
| return await this.gitlabApi.MergeRequests.show( | |
| return this.gitlabApi.MergeRequests.show( |
| const originalSourceBranch = mergeRequest.source_branch; | ||
|
|
||
| if (replayMerged) { | ||
| await this.resetTargetBranchForMergeRequest(mergeRequest, false); |
There was a problem hiding this comment.
Is there a point in keeping the restore bool if it is always set to false?
| if (!mergeRequest.reviewers) return []; | ||
| let reviewers: string[] = []; | ||
| for (let reviewer of mergeRequest.reviewers) { | ||
| let username: string = reviewer.username as string; |
There was a problem hiding this comment.
Type annotation is not needed since it is cast to string.
| let username: string = reviewer.username as string; | |
| let username = reviewer.username as string; |
| if (username === settings.github.username) { | ||
| reviewers.push(settings.github.username); | ||
| } else if (settings.usermap && settings.usermap[username]) { | ||
| let gitHubUsername = settings.usermap[username]; |
There was a problem hiding this comment.
| let gitHubUsername = settings.usermap[username]; | |
| const gitHubUsername = settings.usermap[username]; |
|
|
||
| private async ensureTempBranchForMergeRequest( | ||
| mergeRequest: GitLabMergeRequest | ||
| ): Promise<null | { name: string; created: boolean }> { |
There was a problem hiding this comment.
To be consistent with the other type annotations of the project, { name: string; created: boolean } should be a TS interface.
| const mergedBy = (mergeRequest as any).merged_by?.username; | ||
| const mergedAt = (mergeRequest as any).merged_at; |
There was a problem hiding this comment.
These as any casts should not be needed if the GitLabMergeRequest type is correct.
| static createReviewCommentInformation( | ||
| position, | ||
| repoLink: string | ||
| ): object | null { |
There was a problem hiding this comment.
The change in type here does not do anything: null is an object.
2 participants
No description provided.