Add convenience info to danger.git.diffForFile

changelog.md

@@ -2,7 +2,30 @@
 
 ### Master
 
-* Update internal test fixture generation docs
+* [Enhancements to `danger.git.diffForFile()`](https://github.com/danger/danger-js/pull/223) - @namuol
+
+  * Removed `diffTypes` second argument in favor of `result.added` and `result.removed`
+  * Added `result.before` and `result.after` for easy access to full contents of the original & updated file
+  * `danger.git.diffForFile` is now an `async` function
+
+  #### TL;DR:
+
+    ```js
+    // In danger 0.16.0:
+    const fullDiff = danger.git.diffForFile('foo.js')
+    const addedLines = danger.git.diffForFile('foo.js', ['add'])
+    const removedLines = danger.git.diffForFile('foo.js', ['del'])
+
+    // In the latest version:
+    const diff = await danger.git.diffForFile('foo.js')
+    const fullDiff = diff.diff
+    const addedLines = diff.added
+    const removedLines = diff.removed
+    const beforeFileContents = diff.before
+    const afterFileContents = diff.after
+    ```
+
+* Update internal test fixture generation docs - namuol
 
 ### 0.16.0
 

source/danger.d.ts

@@ -110,6 +110,20 @@ export interface DangerUtilsDSL {
    */
   sentence(array: Array<string>): string
 }
+/** All Text diff values will be this shape */
+export interface TextDiff {
+  /** The value before the PR's applied changes */
+  before: string
+  /** The value after the PR's applied changes */
+  after: string,
+  /** A string containing the full set of changes */
+  diff: string,
+  /** A string containing just the added lines */
+  added: string,
+  /** A string containing just the removed lines */
+  removed: string
+}
+
 /** The results of running a JSON patch */
 export interface JSONPatch {
   /** The JSON in a file at the PR merge base */
@@ -169,10 +183,8 @@ export interface GitDSL {
   /** Offers the diff for a specific file
    *
    * @param {string} filename the path to the json file
-   * @param {string[]} diffTypes An array containing: "add", "del" or "normal" - then you can only get these changes
    */
-
-  diffForFile(filename: string, diffTypes?: string[]): string | null,
+  diffForFile(filename: string): Promise<TextDiff | null>,
 
   /**
    * Provides a JSON patch (rfc6902) between the two versions of a JSON file,

source/dsl/GitDSL.ts

@@ -1,5 +1,19 @@
 import { GitCommit } from "./Commit"
 
+/** All Text diff values will be this shape */
+export interface TextDiff {
+  /** The value before the PR's applied changes */
+  before: string
+  /** The value after the PR's applied changes */
+  after: string,
+  /** A string containing the full set of changes */
+  diff: string,
+  /** A string containing just the added lines */
+  added: string,
+  /** A string containing just the removed lines */
+  removed: string
+}
+
 /** The results of running a JSON patch */
 export interface JSONPatch {
   /** The JSON in a file at the PR merge base */
@@ -62,10 +76,8 @@ export interface GitDSL {
   /** Offers the diff for a specific file
    *
    * @param {string} filename the path to the json file
-   * @param {string[]} diffTypes An array containing: "add", "del" or "normal" - then you can only get these changes
    */
-
-  diffForFile(filename: string, diffTypes?: string[]): string | null,
+  diffForFile(filename: string): Promise<TextDiff | null>,
 
   /**
    * Provides a JSON patch (rfc6902) between the two versions of a JSON file,

source/platforms/FakePlatform.ts

@@ -19,7 +19,7 @@ export class FakePlatform implements Platform {
       modified_files: [],
       created_files: [],
       deleted_files: [],
-      diffForFile: () => "",
+      diffForFile: async () => ({before: "", after: "", diff: "", added: "", removed: ""}),
       JSONDiffForFile: async () => ({} as any),
       JSONPatchForFile: async () => ({} as any),
       commits: []

source/platforms/_tests/fixtures/static_file:98f3e73f5e419f3af9ab928c86312f28a3c87475.json

@@ -0,0 +1,18 @@
+{
+  "name": "tsconfig.json",
+  "path": "tsconfig.json",
+  "sha": "90aabbabf81ac4fbc81ef1e8f40de99972b7e2ac",
+  "size": 153,
+  "url": "https://api.github.com/repos/artsy/emission/contents/tsconfig.json?ref=98f3e73f5e419f3af9ab928c86312f28a3c87475",
+  "html_url": "https://github.com/artsy/emission/blob/98f3e73f5e419f3af9ab928c86312f28a3c87475/tsconfig.json",
+  "git_url": "https://api.github.com/repos/artsy/emission/git/blobs/90aabbabf81ac4fbc81ef1e8f40de99972b7e2ac",
+  "download_url": "https://raw.githubusercontent.com/artsy/emission/98f3e73f5e419f3af9ab928c86312f28a3c87475/tsconfig.json",
+  "type": "file",
+  "content": "ewogICAgImNvbXBpbGVyT3B0aW9ucyI6IHsKICAgICAgICAiYWxsb3dKcyI6\nIHRydWUKICAgIH0sCiAgICAiZXhjbHVkZSI6IFsKICAgICAgICAibm9kZV9t\nb2R1bGVzIiwKICAgICAgICAiUG9kL0Fzc2V0cyIsCiAgICAgICAgIkV4YW1w\nbGUvQnVpbGQiCiAgICBdCn0K\n",
+  "encoding": "base64",
+  "_links": {
+    "self": "https://api.github.com/repos/artsy/emission/contents/tsconfig.json?ref=98f3e73f5e419f3af9ab928c86312f28a3c87475",
+    "git": "https://api.github.com/repos/artsy/emission/git/blobs/90aabbabf81ac4fbc81ef1e8f40de99972b7e2ac",
+    "html": "https://github.com/artsy/emission/blob/98f3e73f5e419f3af9ab928c86312f28a3c87475/tsconfig.json"
+  }
+}

source/platforms/_tests/fixtures/static_file:cfa8fb80d2b65f4c4fa0b54d25352a3a0ff58f75.json

@@ -0,0 +1,18 @@
+{
+  "name": "tsconfig.json",
+  "path": "tsconfig.json",
+  "sha": "c7f3c3d35c0a051b874c043b5ee8d072f3c1434b",
+  "size": 175,
+  "url": "https://api.github.com/repos/artsy/emission/contents/tsconfig.json?ref=cfa8fb80d2b65f4c4fa0b54d25352a3a0ff58f75",
+  "html_url": "https://github.com/artsy/emission/blob/cfa8fb80d2b65f4c4fa0b54d25352a3a0ff58f75/tsconfig.json",
+  "git_url": "https://api.github.com/repos/artsy/emission/git/blobs/c7f3c3d35c0a051b874c043b5ee8d072f3c1434b",
+  "download_url": "https://raw.githubusercontent.com/artsy/emission/cfa8fb80d2b65f4c4fa0b54d25352a3a0ff58f75/tsconfig.json",
+  "type": "file",
+  "content": "ewogICAgImNvbXBpbGVyT3B0aW9ucyI6IHsKICAgICAgICAiYWxsb3dKcyI6\nIHRydWUKICAgIH0sCiAgICAiZXhjbHVkZSI6IFsKICAgICAgICAibm9kZV9t\nb2R1bGVzIiwKICAgICAgICAiUG9kL0Fzc2V0cyIsCiAgICAgICAgIkV4YW1w\nbGUvQnVpbGQiLAogICAgICAgICJleHRlcm5hbHMvIgogICAgXQp9Cg==\n",
+  "encoding": "base64",
+  "_links": {
+    "self": "https://api.github.com/repos/artsy/emission/contents/tsconfig.json?ref=cfa8fb80d2b65f4c4fa0b54d25352a3a0ff58f75",
+    "git": "https://api.github.com/repos/artsy/emission/git/blobs/c7f3c3d35c0a051b874c043b5ee8d072f3c1434b",
+    "html": "https://github.com/artsy/emission/blob/cfa8fb80d2b65f4c4fa0b54d25352a3a0ff58f75/tsconfig.json"
+  }
+}

source/platforms/github/GitHubGit.ts

@@ -12,7 +12,6 @@ import * as parseDiff from "parse-diff"
 import * as includes from "lodash.includes"
 import * as isobject from "lodash.isobject"
 import * as keys from "lodash.keys"
-import * as find from "lodash.find"
 
 import * as jsonDiff from "rfc6902"
 import * as jsonpointer from "jsonpointer"
@@ -64,8 +63,8 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise<GitDSL> {
     if (!modified) { return null }
 
     // Grab the two files contents.
-    const baseFile = await api.fileContents(filename, pr.base.repo.full_name, pr.base.ref)
-    const headFile = await api.fileContents(filename, pr.head.repo.full_name, pr.head.ref)
+    const baseFile = await api.fileContents(filename, pr.base.repo.full_name, pr.base.sha)
+    const headFile = await api.fileContents(filename, pr.head.repo.full_name, pr.head.sha)
 
     if (baseFile && headFile) {
       // Parse JSON
@@ -135,25 +134,32 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise<GitDSL> {
     }, Object.create(null))
   }
 
-/**
- * Gets the git-style diff for a single file.
- *
- * @param name File path for the diff
- */
-  const diffForFile = (name: string, diffTypes?: string[]) => {
-    const diff = find(fileDiffs, (diff: any) => diff.from === name || diff.to === name)
-    if (!diff) { return null }
+  const byType = (t: string) => ({type}: {type: string}) => type === t
+  const getContent = ({content}: {content: string}) => content
 
-    let changes = diff.chunks.map((c: any) => c.changes)
-                             .reduce((a: any, b: any) => a.concat(b), [])
-
-    // Filter the diffs by type
-    if (diffTypes && diffTypes.length > 0) {
-      changes = changes.filter((c: any) => diffTypes.indexOf(c.type) !== -1)
+  type Changes = Array<{type: string, content: string}>
+  /**
+   * Gets the git-style diff for a single file.
+   *
+   * @param filename File path for the diff
+   */
+  const diffForFile = async (filename: string) => {
+    // We already have access to the diff, so see if the file is in there
+    // if it's not return an empty diff
+    const structuredDiff = modifiedDiffs.find((diff: any) => diff.from === filename || diff.to === filename)
+    if (!structuredDiff) { return null }
+
+    const allLines = structuredDiff.chunks
+      .map((c: {changes: Changes}) => c.changes)
+      .reduce((a: Changes, b: Changes) => a.concat(b), [])
+
+    return {
+      before: await api.fileContents(filename, pr.base.repo.full_name, pr.base.sha),
+      after: await api.fileContents(filename, pr.head.repo.full_name, pr.head.sha),
+      diff: allLines.map(getContent).join(os.EOL),
+      added: allLines.filter(byType("add")).map(getContent).join(os.EOL),
+      removed: allLines.filter(byType("del")).map(getContent).join(os.EOL)
     }
-
-    const lines = changes.map((c: any) => c.content)
-    return lines.join(os.EOL)
   }
 
   return {

source/platforms/github/_tests/__snapshots__/_github_git.test.ts.snap

@@ -0,0 +1,11 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`the dangerfile gitDSL should include \`added\` text content of the file 1`] = `"+\\"Example/Build\\",+\\"externals/\\""`;
+
+exports[`the dangerfile gitDSL should include \`after\` text content of the file 1`] = `"{\\"compilerOptions\\":{\\"allowJs\\":true},\\"exclude\\":[\\"node_modules\\",\\"Pod/Assets\\",\\"Example/Build\\",\\"externals/\\"]}"`;
+
+exports[`the dangerfile gitDSL should include \`before\` text content of the file 1`] = `"{\\"compilerOptions\\":{\\"allowJs\\":true},\\"exclude\\":[\\"node_modules\\",\\"Pod/Assets\\",\\"Example/Build\\"]}"`;
+
+exports[`the dangerfile gitDSL should include \`removed\` text content of the file 1`] = `"-\\"Example/Build\\""`;
+
+exports[`the dangerfile gitDSL shows the diff for a specific file 1`] = `"\\"exclude\\":[\\"node_modules\\",\\"Pod/Assets\\",-\\"Example/Build\\"+\\"Example/Build\\",+\\"externals/\\"]}"`;

source/platforms/github/_tests/_github_git.test.ts

@@ -4,31 +4,43 @@ import { GitHubAPI } from "../GitHubAPI"
 import { GitCommit } from "../../../dsl/Commit"
 import { FakeCI } from "../../../ci_source/providers/Fake"
 import { readFileSync } from "fs"
-import { resolve } from "path"
-import * as os from "os"
+import { resolve, join as pathJoin } from "path"
+import { EOL } from "os"
 
 const fixtures = resolve(__dirname, "..", "..", "_tests", "fixtures")
-const EOL = os.EOL
 
 /** Returns JSON from the fixtured dir */
 export const requestWithFixturedJSON = async (path: string): Promise<() => Promise<any>> => () => (
-  Promise.resolve(JSON.parse(readFileSync(`${fixtures}/${path}`, {}).toString()))
+  Promise.resolve(JSON.parse(readFileSync(pathJoin(fixtures, path), {}).toString()))
 )
 
 /** Returns arbitrary text value from a request */
 export const requestWithFixturedContent = async (path: string): Promise<() => Promise<string>> => () => (
-  Promise.resolve(readFileSync(`${fixtures}/${path}`, {}).toString())
+  Promise.resolve(readFileSync(pathJoin(fixtures, path), {}).toString())
 )
 
+/**
+ * HACKish: Jest on Windows seems to include some additional
+ * whitespace that differs from non-Windows snapshot output,
+ * so we strip these until we can better-address the issue.
+ */
+const stripWhitespaceForSnapshot = (str: string) => {
+  return str.replace(/\s/g, "")
+}
+
+const pullRequestInfoFilename = "github_pr.json"
+const masterSHA = JSON.parse(readFileSync(pathJoin(fixtures, pullRequestInfoFilename), {}).toString()).base.sha
+
 describe("the dangerfile gitDSL", async () => {
   let github: GitHub = {} as any
   beforeEach(async () => {
     const api = new GitHubAPI(new FakeCI({}))
     github = new GitHub(api)
 
-    api.getPullRequestInfo = await requestWithFixturedJSON("github_pr.json")
+    api.getPullRequestInfo = await requestWithFixturedJSON(pullRequestInfoFilename)
     api.getPullRequestDiff = await requestWithFixturedContent("github_diff.diff")
     api.getPullRequestCommits = await requestWithFixturedJSON("github_commits.json")
+    api.getFileContents = async (path, repoSlug, ref) => (await requestWithFixturedJSON(`static_file:${ref}.json`))()
   })
 
   it("sets the modified/created/deleted", async () => {
@@ -42,17 +54,45 @@ describe("the dangerfile gitDSL", async () => {
   })
 
   it("shows the diff for a specific file", async () => {
-    const expected = ` - [dev] Updates Flow to 0.32 - orta${EOL} - [dev] Updates React to 0.34 - orta${EOL} - [dev] Turns on "keychain sharing" to fix a keychain bug in sim - orta${EOL}+- GeneVC now shows about information, and trending artists - orta${EOL} ${EOL} ### 1.1.0-beta.2${EOL} ` //tslint:disable-line:max-line-length
     const gitDSL = await github.getPlatformGitRepresentation()
+    const {diff} = await gitDSL.diffForFile("tsconfig.json")
+
+    expect(stripWhitespaceForSnapshot(diff)).toMatchSnapshot()
+  })
+
+  it("should include `before` text content of the file", async () => {
+    const gitDSL = await github.getPlatformGitRepresentation()
+    const {before} = await gitDSL.diffForFile("tsconfig.json")
+
+    expect(stripWhitespaceForSnapshot(before)).toMatchSnapshot()
+  })
+
+  it("should include `after` text content of the file", async () => {
+    const gitDSL = await github.getPlatformGitRepresentation()
+    const {after} = await gitDSL.diffForFile("tsconfig.json")
+
+    expect(stripWhitespaceForSnapshot(after)).toMatchSnapshot()
+  })
+
+  it("should include `added` text content of the file", async () => {
+    const gitDSL = await github.getPlatformGitRepresentation()
+    const {added} = await gitDSL.diffForFile("tsconfig.json")
+
+    expect(stripWhitespaceForSnapshot(added)).toMatchSnapshot()
+  })
+
+  it("should include `removed` text content of the file", async () => {
+    const gitDSL = await github.getPlatformGitRepresentation()
+    const {removed} = await gitDSL.diffForFile("tsconfig.json")
 
-    expect(gitDSL.diffForFile("CHANGELOG.md")).toEqual(expected)
+    expect(stripWhitespaceForSnapshot(removed)).toMatchSnapshot()
   })
 
-  it("should show only diff of specified type", async () => {
-    const expected = "+- GeneVC now shows about information, and trending artists - orta"
+  it("resolves to `null` for files not in modified_files", async () => {
     const gitDSL = await github.getPlatformGitRepresentation()
+    const result = await gitDSL.diffForFile("fuhqmahgads.json")
 
-    expect(gitDSL.diffForFile("CHANGELOG.md", ["add"])).toEqual(expected)
+    expect(result).toBeNull()
   })
 
   it("sets up commit data correctly", async () => {
@@ -101,7 +141,7 @@ describe("the dangerfile gitDSL", async () => {
       }
 
       github.api.fileContents = async (path, repo, ref) => {
-        const obj = (ref === "master") ? before : after
+        const obj = (ref === masterSHA) ? before : after
         return JSON.stringify(obj)
       }
 
@@ -145,7 +185,7 @@ describe("the dangerfile gitDSL", async () => {
           e: ["five", "one", "three"]
         }
 
-        const obj = (ref === "master") ? before : after
+        const obj = (ref === masterSHA) ? before : after
         return JSON.stringify(obj)
       }
       const gitDSL = await github.getPlatformGitRepresentation()
@@ -190,7 +230,7 @@ describe("the dangerfile gitDSL", async () => {
           }
         }
 
-        const obj = (ref === "master") ? before : after
+        const obj = (ref === masterSHA) ? before : after
         return JSON.stringify(obj)
       }
       const gitDSL = await github.getPlatformGitRepresentation()