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

WIP attempt at using the FS beforehand the API for github #991

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ source/danger-outgoing-process-schema.json
source/danger-incoming-process-schema.json
source/platforms/_tests/fixtures/bbs-dsl-input.json
source/platforms/_tests/fixtures/bbc-dsl-input.json
CHANGELOG.md
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

<!-- Your comment below this -->

- Enhancement(perf): [Github] Check if the filesystem can load files instead of always using Github API [#991](https://github.com/danger/danger-js/pull/991) [@orta]
- Fix: [Github] Multiple Inline Comments on the same file/line should all be posted [#1176](https://github.com/danger/danger-js/pull/1176) [@Rouby]

<!-- Your comment above this -->
Expand Down Expand Up @@ -834,7 +835,7 @@ Also, `danger pr` now accepts a `--process` arg.
## 3.5.0 - 3.5.1

- Fixed a bug where Danger posts empty main comment when it have one or more inline comments to post [@codestergit]
- fix bug when commiting .png files on BitBucket [@Mifi]
- fix bug when committing .png files on BitBucket [@Mifi]
- Adds support for inline comments for bitbucket server. [@codestergit]

## 3.4.7
Expand Down
20 changes: 20 additions & 0 deletions source/platforms/git/localGetHeadSHA.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { debug } from "../../debug"
import { exec } from "child_process"

const d = debug("localGetHeadSHA")

export const localGetHeadSHA = () =>
new Promise<string>(done => {
const call = `git rev-parse HEAD`
d(call)

exec(call, (err, stdout, _stderr) => {
if (err) {
console.error(`Could not get the git HEAD for the current path`)
console.error(err)
return
}

done(stdout.trim())
})
})
16 changes: 16 additions & 0 deletions source/platforms/github/GitHubAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@ import { debug } from "../../debug"
import * as node_fetch from "node-fetch"
import parse from "parse-link-header"
import pLimit from "p-limit"
import { existsSync, readFileSync } from "fs"

import { GitHubPRDSL, GitHubIssueComment, GitHubUser } from "../../dsl/GitHubDSL"

import { dangerIDToString } from "../../runner/templates/githubIssueTemplate"
import { api as fetch } from "../../api/fetch"
import { RepoMetaData } from "../../dsl/BitBucketServerDSL"
import { CheckOptions } from "./comms/checks/resultsToCheck"
import { localGetHeadSHA } from "../git/localGetHeadSHA"

// The Handle the API specific parts of the github

Expand All @@ -28,6 +30,7 @@ export class GitHubAPI {
private readonly d = debug("GitHubAPI")

private pr: GitHubPRDSL | undefined
private gitSha: string | undefined

constructor(public readonly repoMetadata: RepoMetaData, public readonly token?: APIToken) {
// This allows Peril to DI in a new Fetch function
Expand Down Expand Up @@ -75,6 +78,19 @@ export class GitHubAPI {
ref = prJSON.head.ref
}

// Only make the check the first time file contents are requested
if (!this.gitSha) {
this.gitSha = await localGetHeadSHA()
}

// See if we can short-cut the API request with a FS lookup
// when we're sure it's on the same setup
if (ref === this.gitSha) {
if (existsSync(path)) {
return readFileSync(path, "utf8")
}
}

const data = await this.getFileContents(path, repoSlug, ref)
const buffer = new Buffer(data.content, "base64")
return buffer.toString()
Expand Down
98 changes: 98 additions & 0 deletions source/platforms/github/getStringDiff.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
function rangesOfDifferBetweenTwoStrings(source: string, target: string) {
const ranges = [] as { start: number; length: number }[]

const addToIndex = (index: number) => {
const closestIndex = ranges[ranges.length - 1]
if (closestIndex) {
const doesAddToIndex = closestIndex.start + closestIndex.length === index - 1
if (doesAddToIndex) {
closestIndex.length = closestIndex.length + 1
} else {
ranges.push({ start: index - 1, length: 1 })
}
} else {
ranges.push({ start: index - 1, length: 1 })
}
}

for (let index = 0; index < source.length; index++) {
const srcChar = source[index]
const targetChar = target[index]
if (srcChar !== targetChar) {
addToIndex(index)
}
}

return ranges
}

function highlightDifferenceBetweenInStrings(source: string, target: string) {
const ranges = rangesOfDifferBetweenTwoStrings(source, target)
let emTarget = target
ranges.forEach((range, index) => {
const lhs = `\e[3m`
const rhs = `\e[0m`
const additionalOffset = index * lhs.length + index * rhs.length
const before = emTarget.slice(0, range.start + 1 + additionalOffset)
const between = emTarget.slice(
range.start + 1 + additionalOffset,
range.start + range.length + 1 + additionalOffset
)
const after = emTarget.slice(range.start + range.length + 1 + additionalOffset, emTarget.length)
emTarget = before + lhs + between + rhs + after
})
return emTarget
}

it("sees a difference", () => {
const src = "Hello world"
const target = "Hello w0rld"
expect(highlightDifferenceBetweenInStrings(src, target)).toMatchInlineSnapshot(`"Hello we[3m0e[0mrld"`)

const src2 = "Hello world"
const target2 = "H3llo w0rld"
expect(highlightDifferenceBetweenInStrings(src2, target2)).toMatchInlineSnapshot(`"He[3m3e[0mllo we[3m0e[0mrld"`)

const src3 = "Hello world ok then"
const target3 = "H3llo w0rld no then"
expect(highlightDifferenceBetweenInStrings(src3, target3)).toMatchInlineSnapshot(
`"He[3m3e[0mllo we[3m0e[0mrld e[3mnoe[0m then"`
)

// expect(rangesOfDifferBetweenTwoStrings(src, target)).toMatchInlineSnapshot(`
// Array [
// Object {
// "length": 1,
// "start": 6,
// },
// ]
// `)

// const multilineInput = `
// ↓
// (↓
// ····<div>↓
// ······text↓
// ····</div>↓
// )
// `
// const multilineOutput = `
// ↓
// (↓
// ····<div>↓
// ········text↓
// ······</div>↓
// )`
// expect(rangesOfDifferBetweenTwoStrings(multilineInput, multilineOutput)).toMatchInlineSnapshot(`
// Array [
// Object {
// "length": 8,
// "start": 22,
// },
// Object {
// "length": 10,
// "start": 32,
// },
// ]
// `)
Comment on lines +62 to +97
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// expect(rangesOfDifferBetweenTwoStrings(src, target)).toMatchInlineSnapshot(`
// Array [
// Object {
// "length": 1,
// "start": 6,
// },
// ]
// `)
// const multilineInput = `
// ↓
// (↓
// ····<div>↓
// ······text↓
// ····</div>↓
// )
// `
// const multilineOutput = `
// ↓
// (↓
// ····<div>↓
// ········text↓
// ······</div>↓
// )`
// expect(rangesOfDifferBetweenTwoStrings(multilineInput, multilineOutput)).toMatchInlineSnapshot(`
// Array [
// Object {
// "length": 8,
// "start": 22,
// },
// Object {
// "length": 10,
// "start": 32,
// },
// ]
// `)

Not sure if this was a copy-pasted test file, or if you actually want these tests to work?

})