From 5f885112c1f4a1a19d67f0409fee6549caa9a42a Mon Sep 17 00:00:00 2001 From: Benny Wong Date: Wed, 6 Sep 2017 22:35:19 -0400 Subject: [PATCH 1/6] GitDSL: Include created and removed files for `diffForFile` * Related to: danger/danger-js#368 --- changelog.md | 2 + source/platforms/github/GitHubGit.ts | 2 +- .../github/_tests/_github_git.test.ts | 84 ++++++++++++++++++- 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/changelog.md b/changelog.md index 949728b2c..7c060416d 100644 --- a/changelog.md +++ b/changelog.md @@ -2,6 +2,8 @@ ### Master +- Updates `diffForFile` to include created and removed files - #368 - bdotdub + ### 2.0.0-alpha.14 - Adds a blank project generated in travis 8 to test no-babel or TS integration - orta diff --git a/source/platforms/github/GitHubGit.ts b/source/platforms/github/GitHubGit.ts index eea4667a9..6b041f29f 100644 --- a/source/platforms/github/GitHubGit.ts +++ b/source/platforms/github/GitHubGit.ts @@ -147,7 +147,7 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise { 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) + const structuredDiff = fileDiffs.find((diff: any) => diff.from === filename || diff.to === filename) if (!structuredDiff) { return null } diff --git a/source/platforms/github/_tests/_github_git.test.ts b/source/platforms/github/_tests/_github_git.test.ts index 6217e46ea..37d30d7f7 100644 --- a/source/platforms/github/_tests/_github_git.test.ts +++ b/source/platforms/github/_tests/_github_git.test.ts @@ -140,7 +140,7 @@ describe("the dangerfile gitDSL", async () => { }) describe("JSONPatchForFile", () => { - it("returns a null for files not in the modified_files", async () => { + it("returns a null for files not in the change list", async () => { const gitDSL = await github.getPlatformGitRepresentation() const empty = await gitDSL.JSONPatchForFile("fuhqmahgads.json") expect(empty).toEqual(null) @@ -179,7 +179,7 @@ describe("the dangerfile gitDSL", async () => { }) describe("JSONDiffForFile", () => { - it("returns an empty object for files not in the modified_files", async () => { + it("returns an empty object for files not in the change list", async () => { const gitDSL = await github.getPlatformGitRepresentation() const empty = await gitDSL.JSONDiffForFile("fuhqmahgads.json") expect(empty).toEqual({}) @@ -278,4 +278,84 @@ describe("the dangerfile gitDSL", async () => { }) }) }) + + describe("textDiffForFile", () => { + it("returns a null for files not in the change list", async () => { + const gitDSL = await github.getPlatformGitRepresentation() + const empty = await gitDSL.diffForFile("fuhqmahgads.json") + expect(empty).toEqual(null) + }) + + it("returns a diff for created files", async () => { + const before = "" + const after = + "/* @flow */\n" + + "'use strict'\n" + + "import Relay from 'react-relay'\n" + + "import React from 'react'\n" + + "import { View, StyleSheet } from 'react-native'\n" + + "import Biography from './biography'\n" + + "import RelatedArtists from '../related_artists'\n" + + "import Separator from '../separator'\n" + + "class About extends React.Component\n" + + github.api.fileContents = async (path, repo, ref) => { + return ref === masterSHA ? before : after + } + + const gitDSL = await github.getPlatformGitRepresentation() + const diff = await gitDSL.diffForFile("lib/components/gene/about.js") + + expect(diff.before).toEqual("") + expect(diff.after).toMatch(/class About extends React.Component/) + expect(diff.diff).toMatch(/class About extends React.Component/) + }) + + it("returns a diff for deleted files", async () => { + const before = + "'use strict'\n" + + "import Relay from 'react-relay'\n" + + "import React from 'react'\n" + + "import { StyleSheet, View, Dimensions } from 'react-native'\n\n" + + "class RelatedArtists extends React.Component" + + const after = "" + + github.api.fileContents = async (path, repo, ref) => { + return ref === masterSHA ? before : after + } + + const gitDSL = await github.getPlatformGitRepresentation() + const diff = await gitDSL.diffForFile("lib/components/artist/related_artists/index.js") + + expect(diff.before).toMatch(/class RelatedArtists extends React.Component/) + expect(diff.after).toEqual("") + expect(diff.diff).toMatch(/class RelatedArtists extends React.Component/) + }) + + it("returns a diff for modified files", async () => { + const before = + `- [dev] Updates Flow to 0.32 - orta\n` + + `- [dev] Updates React to 0.34 - orta\n` + + `- [dev] Turns on "keychain sharing" to fix a keychain bug in sim - orta` + + const after = `- [dev] Updates Flow to 0.32 - orta + - [dev] Updates React to 0.34 - orta + - [dev] Turns on "keychain sharing" to fix a keychain bug in sim - orta + - GeneVC now shows about information, and trending artists - orta` + + github.api.fileContents = async (path, repo, ref) => { + return ref === masterSHA ? before : after + } + + const gitDSL = await github.getPlatformGitRepresentation() + const diff = await gitDSL.diffForFile("CHANGELOG.md") + + expect(diff.before).toEqual(before) + expect(diff.after).toEqual(after) + expect(diff.added).toEqual("+- GeneVC now shows about information, and trending artists - orta") + expect(diff.removed).toEqual("") + expect(diff.diff).toMatch(/GeneVC now shows about information, and trending artists/) + }) + }) }) From a990ba4167e71a3ba4e0b84a31625f1f83ba9f17 Mon Sep 17 00:00:00 2001 From: Benny Wong Date: Thu, 7 Sep 2017 01:29:27 -0400 Subject: [PATCH 2/6] GitDSL: Include created and removed files for `JSONPatchForFile` * Related to: danger/danger-js#368 --- changelog.md | 2 +- source/platforms/github/GitHubGit.ts | 27 +++++---- .../github/_tests/_github_git.test.ts | 56 +++++++++++++++++++ 3 files changed, 70 insertions(+), 15 deletions(-) diff --git a/changelog.md b/changelog.md index 7c060416d..b87c73683 100644 --- a/changelog.md +++ b/changelog.md @@ -2,7 +2,7 @@ ### Master -- Updates `diffForFile` to include created and removed files - #368 - bdotdub +- Updates `diffForFile` and `JSONPatchForFile` to include created and removed files - #368 - bdotdub ### 2.0.0-alpha.14 diff --git a/source/platforms/github/GitHubGit.ts b/source/platforms/github/GitHubGit.ts index 6b041f29f..28b4226a3 100644 --- a/source/platforms/github/GitHubGit.ts +++ b/source/platforms/github/GitHubGit.ts @@ -67,21 +67,20 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise { 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 - const baseJSON = JSON.parse(baseFile) - const headJSON = JSON.parse(headFile) - // Tiny bit of hand-waving here around the types. JSONPatchOperation is - // a simpler version of all operations inside the rfc6902 d.ts. Users - // of danger wont care that much, so I'm smudging the classes slightly - // to be ones we can add to the hosted docs. - return { - before: baseJSON, - after: headJSON, - diff: jsonDiff.createPatch(baseJSON, headJSON) as JSONPatchOperation[], - } + // Parse JSON. `fileContents` returns empty string for files that are + // missing in one of the refs, ie. when the file is created or deleted. + const baseJSON = baseFile === "" ? null : JSON.parse(baseFile) + const headJSON = headFile === "" ? null : JSON.parse(headFile) + + // Tiny bit of hand-waving here around the types. JSONPatchOperation is + // a simpler version of all operations inside the rfc6902 d.ts. Users + // of danger wont care that much, so I'm smudging the classes slightly + // to be ones we can add to the hosted docs. + return { + before: baseJSON, + after: headJSON, + diff: jsonDiff.createPatch(baseJSON, headJSON) as JSONPatchOperation[], } - return null } /** diff --git a/source/platforms/github/_tests/_github_git.test.ts b/source/platforms/github/_tests/_github_git.test.ts index 37d30d7f7..ee603a5cd 100644 --- a/source/platforms/github/_tests/_github_git.test.ts +++ b/source/platforms/github/_tests/_github_git.test.ts @@ -146,6 +146,62 @@ describe("the dangerfile gitDSL", async () => { expect(empty).toEqual(null) }) + it("handles showing a patch for a created file", async () => { + const before = "" + + const after = { + a: "o, world", + b: 3, + c: ["one", "two", "three", "four"], + } + + github.api.fileContents = async (path, repo, ref) => { + const obj = ref === masterSHA ? before : after + return obj === "" ? "" : JSON.stringify(obj) + } + + const gitDSL = await github.getPlatformGitRepresentation() + const empty = await gitDSL.JSONPatchForFile("data/schema.json") + expect(empty).toEqual({ + before: null, + after: after, + diff: [ + { + op: "replace", + path: "", + value: { + a: "o, world", + b: 3, + c: ["one", "two", "three", "four"], + }, + }, + ], + }) + }) + + it("handles showing a patch for a deleted file", async () => { + const before = { + a: "o, world", + b: 3, + c: ["one", "two", "three", "four"], + } + + const after = "" + + github.api.fileContents = async (path, repo, ref) => { + const obj = ref === masterSHA ? before : after + return obj === "" ? "" : JSON.stringify(obj) + } + + const gitDSL = await github.getPlatformGitRepresentation() + const empty = await gitDSL.JSONPatchForFile("data/schema.json") + expect(empty).toEqual({ + before: before, + after: null, + diff: [{ op: "replace", path: "", value: null }], + }) + }) + it("handles showing a patch for two different diff files", async () => { const before = { a: "Hello, world", From 95bac1b10870f0422858276c3b81750ef82adce3 Mon Sep 17 00:00:00 2001 From: Benny Wong Date: Thu, 7 Sep 2017 18:46:58 -0400 Subject: [PATCH 3/6] GitDSL: Handle created and deleted files semantically consistent * Discussion: https://github.com/danger/danger-js/pull/369#discussion_r137557557 --- source/platforms/github/GitHubGit.ts | 8 ++++---- source/platforms/github/_tests/_github_git.test.ts | 14 ++++---------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/source/platforms/github/GitHubGit.ts b/source/platforms/github/GitHubGit.ts index 28b4226a3..fdf4bf274 100644 --- a/source/platforms/github/GitHubGit.ts +++ b/source/platforms/github/GitHubGit.ts @@ -69,16 +69,16 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise { // Parse JSON. `fileContents` returns empty string for files that are // missing in one of the refs, ie. when the file is created or deleted. - const baseJSON = baseFile === "" ? null : JSON.parse(baseFile) - const headJSON = headFile === "" ? null : JSON.parse(headFile) + const baseJSON = baseFile === "" ? {} : JSON.parse(baseFile) + const headJSON = headFile === "" ? {} : JSON.parse(headFile) // Tiny bit of hand-waving here around the types. JSONPatchOperation is // a simpler version of all operations inside the rfc6902 d.ts. Users // of danger wont care that much, so I'm smudging the classes slightly // to be ones we can add to the hosted docs. return { - before: baseJSON, - after: headJSON, + before: baseFile === "" ? null : baseJSON, + after: headFile === "" ? null : headJSON, diff: jsonDiff.createPatch(baseJSON, headJSON) as JSONPatchOperation[], } } diff --git a/source/platforms/github/_tests/_github_git.test.ts b/source/platforms/github/_tests/_github_git.test.ts index ee603a5cd..e9e1e3961 100644 --- a/source/platforms/github/_tests/_github_git.test.ts +++ b/source/platforms/github/_tests/_github_git.test.ts @@ -166,15 +166,9 @@ describe("the dangerfile gitDSL", async () => { before: null, after: after, diff: [ - { - op: "replace", - path: "", - value: { - a: "o, world", - b: 3, - c: ["one", "two", "three", "four"], - }, - }, + { op: "add", path: "/a", value: "o, world" }, + { op: "add", path: "/b", value: 3 }, + { op: "add", path: "/c", value: ["one", "two", "three", "four"] }, ], }) }) @@ -198,7 +192,7 @@ describe("the dangerfile gitDSL", async () => { expect(empty).toEqual({ before: before, after: null, - diff: [{ op: "replace", path: "", value: null }], + diff: [{ op: "remove", path: "/a" }, { op: "remove", path: "/b" }, { op: "remove", path: "/c" }], }) }) From 058528e862c01f1ba1f566b8f4ecef7035fba34d Mon Sep 17 00:00:00 2001 From: Benny Wong Date: Thu, 7 Sep 2017 22:55:28 -0400 Subject: [PATCH 4/6] GitDSL: Include created and removed files for `JSONDiffForFile` * Related to: danger/danger-js#368 --- changelog.md | 2 +- source/platforms/github/GitHubGit.ts | 29 +++++++---- .../github/_tests/_github_git.test.ts | 48 +++++++++++++++++++ 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/changelog.md b/changelog.md index b87c73683..5352ed708 100644 --- a/changelog.md +++ b/changelog.md @@ -2,7 +2,7 @@ ### Master -- Updates `diffForFile` and `JSONPatchForFile` to include created and removed files - #368 - bdotdub +- Updates `diffForFile`, `JSONPatchForFile`, and `JSONDiffForFile` to include created and removed files - #368 - bdotdub ### 2.0.0-alpha.14 diff --git a/source/platforms/github/GitHubGit.ts b/source/platforms/github/GitHubGit.ts index fdf4bf274..999fe9294 100644 --- a/source/platforms/github/GitHubGit.ts +++ b/source/platforms/github/GitHubGit.ts @@ -106,25 +106,36 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise { const backAStepPath = pathSteps.length <= 2 ? path : pathSteps.slice(0, pathSteps.length - 1).join("/") const diff: any = { - after: jsonpointer.get(after, backAStepPath), - before: jsonpointer.get(before, backAStepPath), + after: jsonpointer.get(after, backAStepPath) || null, + before: jsonpointer.get(before, backAStepPath) || null, } + const counterpart = (other) => { + if (Array.isArray(other)) { + return [] + } else if (isobject(diff.after)) { + return {} + } + return null + } + + const beforeValue = diff.before || counterpart(diff.after) + const afterValue = diff.after || counterpart(diff.before) + // If they both are arrays, add some extra metadata about what was // added or removed. This makes it really easy to act on specific // changes to JSON DSLs - if (Array.isArray(diff.after) && Array.isArray(diff.before)) { - const arrayBefore = diff.before as any[] - const arrayAfter = diff.after as any[] + if (Array.isArray(afterValue) && Array.isArray(beforeValue)) { + const arrayBefore = beforeValue as any[] + const arrayAfter = afterValue as any[] diff.added = arrayAfter.filter(o => !includes(arrayBefore, o)) diff.removed = arrayBefore.filter(o => !includes(arrayAfter, o)) - // Do the same, but for keys inside an object if they both are objects. - } else if (isobject(diff.after) && isobject(diff.before)) { - const beforeKeys = keys(diff.before) as string[] - const afterKeys = keys(diff.after) as string[] + } else if (isobject(afterValue) && isobject(beforeValue)) { + const beforeKeys = keys(beforeValue) as string[] + const afterKeys = keys(afterValue) as string[] diff.added = afterKeys.filter(o => !includes(beforeKeys, o)) diff.removed = beforeKeys.filter(o => !includes(afterKeys, o)) } diff --git a/source/platforms/github/_tests/_github_git.test.ts b/source/platforms/github/_tests/_github_git.test.ts index e9e1e3961..21ad4a734 100644 --- a/source/platforms/github/_tests/_github_git.test.ts +++ b/source/platforms/github/_tests/_github_git.test.ts @@ -235,6 +235,54 @@ describe("the dangerfile gitDSL", async () => { expect(empty).toEqual({}) }) + it("handles showing a patch for a created file", async () => { + github.api.fileContents = async (path, repo, ref) => { + const after = { + a: "o, world", + b: 3, + c: ["one", "two", "three", "four"], + d: ["one", "two"], + e: ["five", "one", "three"], + } + + const obj = ref === masterSHA ? {} : after + return JSON.stringify(obj) + } + const gitDSL = await github.getPlatformGitRepresentation() + const empty = await gitDSL.JSONDiffForFile("data/schema.json") + expect(empty).toEqual({ + a: { after: "o, world", before: null }, + b: { after: 3, before: null }, + c: { added: ["one", "two", "three", "four"], after: ["one", "two", "three", "four"], before: null, removed: [] }, + d: { added: ["one", "two"], after: ["one", "two"], before: null, removed: [] }, + e: { added: ["five", "one", "three"], after: ["five", "one", "three"], before: null, removed: [] }, + }) + }) + + it("handles showing a patch for a deleted file", async () => { + github.api.fileContents = async (path, repo, ref) => { + const before = { + a: "o, world", + b: 3, + c: ["one", "two", "three", "four"], + d: ["one", "two"], + e: ["five", "one", "three"], + } + + const obj = ref === masterSHA ? before : {} + return JSON.stringify(obj) + } + const gitDSL = await github.getPlatformGitRepresentation() + const empty = await gitDSL.JSONDiffForFile("data/schema.json") + expect(empty).toEqual({ + a: { after: null, before: "o, world" }, + b: { after: null, before: 3 }, + c: { added: [], after: null, before: ["one", "two", "three", "four"], removed: ["one", "two", "three", "four"] }, + d: { added: [], after: null, before: ["one", "two",], removed: ["one", "two"] }, + e: { added: [], after: null, before: ["five", "one", "three"], removed: ["five", "one", "three"] }, + }) + }) + it("handles showing a patch for two different diff files", async () => { github.api.fileContents = async (path, repo, ref) => { const before = { From 9eea00c88e5ca96286bfc7218d36f082fb727d7b Mon Sep 17 00:00:00 2001 From: Benny Wong Date: Thu, 7 Sep 2017 23:08:38 -0400 Subject: [PATCH 5/6] GitDSL: Update function to a more descriptive name --- source/platforms/github/GitHubGit.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/platforms/github/GitHubGit.ts b/source/platforms/github/GitHubGit.ts index 999fe9294..ac6dd4d32 100644 --- a/source/platforms/github/GitHubGit.ts +++ b/source/platforms/github/GitHubGit.ts @@ -110,7 +110,7 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise { before: jsonpointer.get(before, backAStepPath) || null, } - const counterpart = (other) => { + const emptyValueOfCounterpart = (other: any) => { if (Array.isArray(other)) { return [] } else if (isobject(diff.after)) { @@ -119,8 +119,8 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise { return null } - const beforeValue = diff.before || counterpart(diff.after) - const afterValue = diff.after || counterpart(diff.before) + const beforeValue = diff.before || emptyValueOfCounterpart(diff.after) + const afterValue = diff.after || emptyValueOfCounterpart(diff.before) // If they both are arrays, add some extra metadata about what was // added or removed. This makes it really easy to act on specific From 1562db487e7a9f73d012b6423766be5f913d42c9 Mon Sep 17 00:00:00 2001 From: Benny Wong Date: Thu, 7 Sep 2017 23:11:41 -0400 Subject: [PATCH 6/6] Lint: Fix --- .../platforms/github/_tests/_github_git.test.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/source/platforms/github/_tests/_github_git.test.ts b/source/platforms/github/_tests/_github_git.test.ts index 21ad4a734..3e59cba4f 100644 --- a/source/platforms/github/_tests/_github_git.test.ts +++ b/source/platforms/github/_tests/_github_git.test.ts @@ -253,7 +253,12 @@ describe("the dangerfile gitDSL", async () => { expect(empty).toEqual({ a: { after: "o, world", before: null }, b: { after: 3, before: null }, - c: { added: ["one", "two", "three", "four"], after: ["one", "two", "three", "four"], before: null, removed: [] }, + c: { + added: ["one", "two", "three", "four"], + after: ["one", "two", "three", "four"], + before: null, + removed: [], + }, d: { added: ["one", "two"], after: ["one", "two"], before: null, removed: [] }, e: { added: ["five", "one", "three"], after: ["five", "one", "three"], before: null, removed: [] }, }) @@ -277,8 +282,13 @@ describe("the dangerfile gitDSL", async () => { expect(empty).toEqual({ a: { after: null, before: "o, world" }, b: { after: null, before: 3 }, - c: { added: [], after: null, before: ["one", "two", "three", "four"], removed: ["one", "two", "three", "four"] }, - d: { added: [], after: null, before: ["one", "two",], removed: ["one", "two"] }, + c: { + added: [], + after: null, + before: ["one", "two", "three", "four"], + removed: ["one", "two", "three", "four"], + }, + d: { added: [], after: null, before: ["one", "two"], removed: ["one", "two"] }, e: { added: [], after: null, before: ["five", "one", "three"], removed: ["five", "one", "three"] }, }) })