Skip to content

Commit

Permalink
First version of updating inline comments
Browse files Browse the repository at this point in the history
  • Loading branch information
sunshinejr committed Mar 17, 2018
1 parent c9c3d52 commit 26dcf50
Show file tree
Hide file tree
Showing 13 changed files with 161 additions and 66 deletions.
11 changes: 0 additions & 11 deletions source/dsl/GitHubDSL.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,14 +368,3 @@ export interface GitHubAPIPR {
/** The PR number */
number: number
}

export interface GitHubComment {
/** The identifying number of this comment */
id: number

/** The identifying number of this comment's author */
userId: number

/** Body of this comment in a text form */
body: string
}
23 changes: 18 additions & 5 deletions source/platforms/BitBucketServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { GitJSONDSL, GitDSL } from "../dsl/GitDSL"
import { BitBucketServerPRDSL, BitBucketServerJSONDSL } from "../dsl/BitBucketServerDSL"
import { BitBucketServerAPI } from "./bitbucket_server/BitBucketServerAPI"
import gitDSLForBitBucketServer from "./bitbucket_server/BitBucketServerGit"
import { Platform } from "./platform"
import { Platform, Comment } from "./platform"

/** Handles conforming to the Platform Interface for BitBucketServer, API work is handle by BitBucketServerAPI */

Expand All @@ -27,6 +27,11 @@ export class BitBucketServer implements Platform {
*/
getPlatformGitRepresentation = (): Promise<GitJSONDSL> => gitDSLForBitBucketServer(this.api)

/**
* Gets inline comments for current PR
*/
getInlineComments = async (): Promise<Comment[]> => new Promise<Comment[]>((_resolve, reject) => reject())

/**
* Fails the current build, if status setting succeeds
* then return true.
Expand Down Expand Up @@ -99,13 +104,21 @@ export class BitBucketServer implements Platform {

/**
* Makes an inline comment if possible. If platform can't make an inline comment with given arguments,
* it returns `undefined`. (e.g. platform doesn't support inline comments or line was out of diff).
* it returns a promise rejection. (e.g. platform doesn't support inline comments or line was out of diff).
*
* @returns {Promise<any>} JSON response of new comment
*/
createInlineComment = (_git: GitDSL, _comment: string, _path: string, _line: number): Promise<any> => {
return new Promise((_resolve, reject) => reject())
}
createInlineComment = (_git: GitDSL, _comment: string, _path: string, _line: number): Promise<any> =>
new Promise((_resolve, reject) => reject())

/**
* Updates an inline comment if possible. If platform can't update an inline comment,
* it returns a promise rejection. (e.g. platform doesn't support inline comments or line was out of diff).
*
* @returns {Promise<any>} JSON response of new comment
*/
updateInlineComment = (_comment: string, _commentId: string): Promise<any> =>
new Promise((_resolve, reject) => reject())

/**
* Deletes the main Danger comment, used when you have
Expand Down
10 changes: 9 additions & 1 deletion source/platforms/FakePlatform.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { GitDSL } from "../dsl/GitDSL"
import { CISource } from "../ci_source/ci_source"
import { Platform } from "./platform"
import { Platform, Comment } from "./platform"
import { readFileSync } from "fs-extra"

export class FakePlatform implements Platform {
Expand Down Expand Up @@ -36,6 +36,10 @@ export class FakePlatform implements Platform {
}
}

async getInlineComments(): Promise<Comment[]> {
return []
}

supportsCommenting() {
return true
}
Expand All @@ -56,6 +60,10 @@ export class FakePlatform implements Platform {
return true
}

async updateInlineComment(_comment: string, _commentId: string): Promise<any> {
return true
}

async deleteMainComment(): Promise<boolean> {
return true
}
Expand Down
34 changes: 24 additions & 10 deletions source/platforms/GitHub.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { GitJSONDSL, GitDSL } from "../dsl/GitDSL"
import { GitHubPRDSL, GitHubDSL, GitHubIssue, GitHubAPIPR, GitHubJSONDSL, GitHubComment } from "../dsl/GitHubDSL"
import { GitHubPRDSL, GitHubDSL, GitHubIssue, GitHubAPIPR, GitHubJSONDSL } from "../dsl/GitHubDSL"
import { GitHubAPI } from "./github/GitHubAPI"
import GitHubUtils from "./github/GitHubUtils"
import gitDSLForGitHub from "./github/GitHubGit"

import * as NodeGitHub from "@octokit/rest"
import { Platform } from "./platform"
import { Platform, Comment } from "./platform"

/** Handles conforming to the Platform Interface for GitHub, API work is handle by GitHubAPI */

Expand Down Expand Up @@ -42,13 +42,7 @@ export class GitHub implements Platform {
/**
* Gets inline comments for current PR
*/
getInlineComments = async (): Promise<GitHubComment[]> =>
this.api.getPullRequestInlineComments().then(v => {
return v.map(i => {
// End user doesn't really need more than id/userId/body right now
return { id: i.id, userId: i.user.id, body: i.body }
})
})
getInlineComments = async (): Promise<Comment[]> => this.api.getPullRequestInlineComments()

/**
* Fails the current build, if status setting succeeds
Expand Down Expand Up @@ -128,7 +122,7 @@ export class GitHub implements Platform {

/**
* Makes an inline comment if possible. If platform can't make an inline comment with given arguments,
* it returns `undefined`. (e.g. platform doesn't support inline comments or line was out of diff).
* it returns a promise rejection. (e.g. platform doesn't support inline comments or line was out of diff).
*
* @returns {Promise<any>} JSON response of new comment
*/
Expand All @@ -144,6 +138,26 @@ export class GitHub implements Platform {
})
}

/**
* Updates an inline comment if possible. If platform can't update an inline comment,
* it returns a promise rejection. (e.g. platform doesn't support inline comments or line was out of diff).
*
* @returns {Promise<any>} JSON response of new comment
*/
updateInlineComment = (comment: string, commentId: string): Promise<any> => {
if (!this.supportsInlineComments) {
return new Promise((_resolve, reject) => reject())
}

return this.api.updateInlinePRComment(comment, commentId)
}

/**
* Finds a position in given diff. This is needed for GitHub API, more on the position finder
* can be found here: https://developer.github.com/v3/pulls/comments/#create-a-comment
*
* @returns {Promise<number>} A number with given position
*/
findPositionForInlineComment = (git: GitDSL, line: number, path: string): Promise<number> => {
return git.diffForFile(path).then(diff => {
return new Promise<number>((resolve, reject) => {
Expand Down
10 changes: 9 additions & 1 deletion source/platforms/LocalGit.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { GitDSL } from "../dsl/GitDSL"
import { Platform } from "./platform"
import { Platform, Comment } from "./platform"
import { gitJSONToGitDSL, GitJSONToGitDSLConfig } from "./git/gitJSONToGitDSL"
import { diffToGitJSONDSL } from "./git/diffToGitJSONDSL"
import { GitCommit } from "../dsl/Commit"
Expand Down Expand Up @@ -59,6 +59,10 @@ export class LocalGit implements Platform {
return gitJSONToGitDSL(gitJSON, config)
}

async getInlineComments(): Promise<Comment[]> {
return []
}

supportsCommenting() {
return false
}
Expand All @@ -79,6 +83,10 @@ export class LocalGit implements Platform {
return true
}

async updateInlineComment(_comment: string, _commentId: string): Promise<any> {
return true
}

async deleteMainComment(): Promise<boolean> {
return true
}
Expand Down
1 change: 0 additions & 1 deletion source/platforms/_tests/_github.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ describe("getPlatformDSLRepresentation", () => {
it("should get the inline comments for this PR", async () => {
const comments = await github.getInlineComments()
expect(comments[0].id).toEqual(81345954)
expect(comments[0].userId).toEqual(49038)
expect(comments[0].body).toEqual("needed to update the schema for description\n")
expect(comments.length).toEqual(6)
})
Expand Down
26 changes: 24 additions & 2 deletions source/platforms/github/GitHubAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { GitHubPRDSL, GitHubUser } from "../../dsl/GitHubDSL"
import { RepoMetaData } from "../../ci_source/ci_source"
import { dangerSignaturePostfix, dangerIDToString } from "../../runner/templates/githubIssueTemplate"
import { api as fetch } from "../../api/fetch"
import { Comment } from "../platform"

// The Handle the API specific parts of the github

Expand Down Expand Up @@ -159,6 +160,22 @@ export class GitHubAPI {
}
}

updateInlinePRComment = async (comment: string, commentId: string) => {
const repo = this.repoMetadata.repoSlug
const res = await this.patch(
`repos/${repo}/pulls/comments/${commentId}`,
{},
{
body: comment,
}
)
if (res.ok) {
return res.json()
} else {
throw await res.json()
}
}

getPullRequestInfo = async (): Promise<GitHubPRDSL> => {
if (this.pr) {
return this.pr
Expand Down Expand Up @@ -247,10 +264,15 @@ export class GitHubAPI {
return await this.getAllOfResource(`repos/${repo}/issues/${prID}/comments`)
}

getPullRequestInlineComments = async (): Promise<any[]> => {
getPullRequestInlineComments = async (): Promise<Comment[]> => {
const userID = await this.getUserID()
const repo = this.repoMetadata.repoSlug
const prID = this.repoMetadata.pullRequestID
return await this.getAllOfResource(`repos/${repo}/pulls/${prID}/comments`)
return await this.getAllOfResource(`repos/${repo}/pulls/${prID}/comments`).then(v => {
return v.map((i: any) => {
return { id: i.id, ownedByDanger: i.user.id == userID, body: i.body }
})
})
}

getPullRequestDiff = async () => {
Expand Down
4 changes: 4 additions & 0 deletions source/platforms/platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ export interface Platform {
getPlatformDSLRepresentation: () => Promise<any>
/** Pulls in the Code Review Diff, and offers a succinct user-API for it */
getPlatformGitRepresentation: () => Promise<GitJSONDSL>
/** Gets inline comments for current PR */
getInlineComments: () => Promise<Comment[]>
/** Can it update comments? */
supportsCommenting: () => boolean
/** Does the platform support inline comments? */
Expand All @@ -47,6 +49,8 @@ export interface Platform {
// Here we pass GitDSL because platforms have different endpoints for inline comments
// Wasn't sure if passing the dsl is the best way of achieving this, though
createInlineComment: (git: GitDSL, comment: string, path: string, line: number) => Promise<any>
/** Updates an inline comment with given id */
updateInlineComment: (comment: string, commentId: string) => Promise<any>
/** Delete the main Danger comment */
deleteMainComment: (dangerID: string) => Promise<boolean>
/** Replace the main Danger comment */
Expand Down
64 changes: 49 additions & 15 deletions source/runner/Executor.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { contextForDanger, DangerContext } from "./Dangerfile"
import { CISource } from "../ci_source/ci_source"
import { Platform } from "../platforms/platform"
import { Platform, Comment } from "../platforms/platform"
import {
inlineResults,
regularResults,
Expand All @@ -16,6 +16,7 @@ import {
import {
template as githubResultsTemplate,
inlineTemplate as githubResultsInlineTemplate,
fileLineToString,
} from "./templates/githubIssueTemplate"
import {
template as bitbucketServerTemplate,
Expand Down Expand Up @@ -215,8 +216,9 @@ export class Executor {
} else if (messageCount > 0) {
console.log("Found only messages, passing those to review.")
}
const previousComments = await this.platform.getInlineComments()
const inline = inlineResults(results)
const inlineLeftovers = await this.sendInlineComments(inline, git)
const inlineLeftovers = await this.sendInlineComments(inline, git, previousComments)
const regular = regularResults(results)
const mergedResults = sortResults(mergeResults(regular, inlineLeftovers))
const comment = process.env["DANGER_BITBUCKETSERVER_HOST"]
Expand All @@ -238,37 +240,69 @@ export class Executor {
*
* @param results Results with inline violations
*/
sendInlineComments(results: DangerResults, git: GitDSL): Promise<DangerResults> {
// TODO: Get current inline comments, if any of the old ones is not present
// in the new ones - delete.
sendInlineComments(results: DangerResults, git: GitDSL, previousComments: Comment[]): Promise<DangerResults> {
if (!this.platform.supportsInlineComments) {
return new Promise(resolve => resolve(results))
}

const inlineResults = resultsIntoInlineResults(results)
const sortedInlineResults = sortInlineResults(inlineResults)
const promises = sortedInlineResults.map(inlineResult => {
return this.sendInlineComment(git, inlineResult)
.then(_r => emptyDangerResults)
.catch(_e => inlineResultsIntoResults(inlineResult))
})
return Promise.all(promises).then(dangerResults => {

// For every inline result check if there is a comment already
// if there is - update it and remove comment from deleteComments array (comments prepared for deletion)
// if there isn't - create a new comment
// Leftovers in deleteComments array should all be deleted afterwards
let deleteComments = previousComments.filter(c => c.ownedByDanger)
let commentPromises: Promise<any>[] = []
for (let inlineResult of sortedInlineResults) {
const index = deleteComments.findIndex(p =>
p.body.includes(fileLineToString(inlineResult.file, inlineResult.line))
)
let promise: Promise<any>
if (index != -1) {
let previousComment = deleteComments[index]
delete deleteComments[index]
promise = this.updateInlineComment(inlineResult, previousComment)
} else {
promise = this.sendInlineComment(git, inlineResult)
}
commentPromises.push(promise.then(_r => emptyDangerResults).catch(_e => inlineResultsIntoResults(inlineResult)))
}
// TODO: Remove leftovers from deleteComments array

return Promise.all(commentPromises).then(dangerResults => {
return new Promise<DangerResults>(resolve => {
resolve(dangerResults.reduce((acc, r) => mergeResults(acc, r), emptyDangerResults))
})
})
}

async sendInlineComment(git: GitDSL, inlineResults: DangerInlineResults): Promise<any> {
if (!this.platform.supportsInlineComments) {
const comment = this.inlineCommentTemplate(inlineResults)
return await this.platform.createInlineComment(git, comment, inlineResults.file, inlineResults.line)
}

async updateInlineComment(inlineResults: DangerInlineResults, previousComment: Comment): Promise<any> {
const body = this.inlineCommentTemplate(inlineResults)
// If generated body is exactly the same as current comment we don't send an API request
if (body == previousComment.body) {
return
}

return await this.platform.updateInlineComment(body, previousComment.id)
}

inlineCommentTemplate(inlineResults: DangerInlineResults): string {
const results = inlineResultsIntoResults(inlineResults)
const comment = process.env["DANGER_BITBUCKETSERVER_HOST"]
? bitbucketServerInlineTemplate(results)
: githubResultsInlineTemplate(this.options.dangerID, results)
return await this.platform.createInlineComment(git, comment, inlineResults.file, inlineResults.line)
: githubResultsInlineTemplate(this.options.dangerID, results, inlineResults.file, inlineResults.line)

return comment
}

/**
* Takes an error (maybe a bad eval) and provides a DangerResults compatible object
* Takes an error (maybe a bad eval) and provides a DangerResults compatible object3ehguh.l;/////////////
* @param error Any JS error
*/
resultsForError(error: Error) {
Expand Down

0 comments on commit 26dcf50

Please sign in to comment.