Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include created and removed files for diffForFile, JSONDiffForFile, JSONPatchForFile #369

Merged
merged 6 commits into from
Sep 8, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

### Master

- Updates `diffForFile` and `JSONPatchForFile` 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
Expand Down
29 changes: 14 additions & 15 deletions source/platforms/github/GitHubGit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,21 +67,20 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise<GitDSL> {
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
}

/**
Expand Down Expand Up @@ -147,7 +146,7 @@ export default async function gitDSLForGitHub(api: GitHubAPI): Promise<GitDSL> {
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
}
Expand Down
140 changes: 138 additions & 2 deletions source/platforms/github/_tests/_github_git.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,68 @@ 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)
})

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: [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orta Would love to get your thoughts here.

For created files, I think it makes sense to have before return null since the before is the absence of the file. The diff is technically correct to, but feel unexpected and/or inconsistent with other behavior.

What do you think about JSONPatchForFile returning null for before, but the diff as the result between {} and after?

So instead of this:

{
  "op": "replace",
  "path": "",
  "value": {
    "a": "o, world",
    "b": 3,
    "c": ["one", "two", "three", "four"],
  }
}

it becomes this:

[
  {
    "op": "add",
    "path": "/a",
    "value": "o, world"
  },
  {
    "op": "add",
    "path": "/b",
    "value": 3
  },
  {
    "op": "add",
    "path": "/c",
    "value": [
      "one",
      "two",
      "three",
      "four"
    ]
  }
]

While not technically correct, I think it makes more sense semantically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree 👍

{
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",
Expand Down Expand Up @@ -179,7 +235,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({})
Expand Down Expand Up @@ -278,4 +334,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/)
})
})
})