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

Add convenience info to `danger.git.diffForFile` #223

Merged
merged 13 commits into from Apr 15, 2017

Conversation

@namuol
Copy link
Member

@namuol namuol commented Apr 13, 2017

Fixes #221

Changes the signature for danger.git.diffForFile to the following:

/** 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
}

diffForFile(filename: string): Promise<TextDiff | null>

TODO

  • Update docs
  • Update changelog (include dangerfile migration instructions)
  • Run yarn declarations
  • Add tests
    • before
    • after
    • added
    • removed
  • Fix snapshot issues (?)
@@ -64,8 +76,7 @@ export interface GitDSL {
* @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, diffTypes?: string[]): Promise<TextDiff | null>,

This comment has been minimized.

@orta

orta Apr 13, 2017
Member

think you can kill diffTypes in favour of this /cc @alex3165

This comment has been minimized.

@namuol

namuol Apr 13, 2017
Author Member

Should we still keep the added/removed/normal changes? They're not really the same as JSONDiff.

This comment has been minimized.

@orta

orta Apr 13, 2017
Member

Can't they be the raw string content of all the hunks combined?

This comment has been minimized.

@namuol

namuol Apr 13, 2017
Author Member

Ah, yes that does make sense. @alex3165 does this satisfy your use-case?

i.e. instead of calling diffForFile('foo.js', ['add']), you'd call diffForFile('foo.js').added which would return the same sort of string.

This comment has been minimized.

@namuol

namuol Apr 13, 2017
Author Member

I honestly am confused about the meaning of normal in this context -- I think it refers to "contextual" lines (i.e. non-changed lines that sandwich an addition/deletion).

If I'm correct about this, I struggle to see how it'd be useful to have these lines without being combined with added or removed.

Maybe we could just provide the raw structuredDiff from parseDiff's output as an escape hatch for folks who need finer control.

This comment has been minimized.

@orta

orta Apr 13, 2017
Member

maybe then added and removed should be add + normal and del?

This comment has been minimized.

@namuol

namuol Apr 13, 2017
Author Member

After some thinking, here's what I think would be more useful for most folks:

/** 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,
}

This covers all cases except for the odd one where you only want normal/contextual lines, which seems quite fringe to me.

This comment has been minimized.

@orta

orta Apr 13, 2017
Member

Yeah, that feels 👍 to me

removed: changes.filter(({type}: {type: string}) => type === "del"),
normal: changes.filter(({type}: {type: string}) => type === "normal"),
diff: lines.join(os.EOL)
}
}

This comment has been minimized.

@orta

orta Apr 13, 2017
Member

👍

@codecov-io
Copy link

@codecov-io codecov-io commented Apr 13, 2017

Codecov Report

Merging #223 into master will not change coverage.
The diff coverage is 92.3%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #223   +/-   ##
=======================================
  Coverage   68.62%   68.62%           
=======================================
  Files          34       34           
  Lines         765      765           
  Branches      105      104    -1     
=======================================
  Hits          525      525           
  Misses        240      240
Impacted Files Coverage Δ
source/platforms/FakePlatform.ts 68.42% <0%> (ø) ⬆️
source/platforms/github/GitHubGit.ts 98.27% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3f1933...82b089c. Read the comment docs.


// Filter the diffs by type
if (diffTypes && diffTypes.length > 0) {
changes = changes.filter((c: any) => diffTypes.indexOf(c.type) !== -1)
}

const baseFile = await api.fileContents(filename, pr.base.repo.full_name, pr.base.sha)

This comment has been minimized.

@namuol

namuol Apr 13, 2017
Author Member

Note: I'm using sha here instead of ref, mainly because the example PR used for the fixture seemed to have deleted the original branch.

We may also want to use sha above in the implementation of JSONPatchForFile.

@orta Thoughts? Any reason to prefer ref over sha?

This comment has been minimized.

@orta

orta Apr 13, 2017
Member

no difference IMO 👍

This comment has been minimized.

@orta

orta Apr 13, 2017
Member

sha may be better incase a potential branch has had new commits since the CI was started, and then when danger runs

@namuol
Copy link
Member Author

@namuol namuol commented Apr 13, 2017

Hmm... I must be missing something. The tests are passing on my machine but AppVeyor doesn't seem to like it. 🤷‍♂️

@@ -20,15 +20,19 @@ export const requestWithFixturedContent = async (path: string): Promise<() => Pr
Promise.resolve(readFileSync(`${fixtures}/${path}`, {}).toString())
)

const pullRequestInfoFilename = "github_pr.json"
const masterSHA = JSON.parse(readFileSync(`${fixtures}/${pullRequestInfoFilename}`, {}).toString()).base.sha

This comment has been minimized.

@orta

orta Apr 14, 2017
Member

try using the path API for this string - that might be what's breaking on windows, can't remember its name yet

This comment has been minimized.

@namuol

namuol Apr 14, 2017
Author Member

This could possibly be a bug with Jest's snapshot testing. :-/ Their Windows support is still pretty unstable AFAIK.

This comment has been minimized.

@namuol

namuol Apr 14, 2017
Author Member

I thought maybe it had something to do with line endings, but that shouldn't be an issue: facebook/jest#1879

@orta
Copy link
Member

@orta orta commented Apr 14, 2017

These failing snapshots feel backwards, like it's inverting the before and after

@namuol
Copy link
Member Author

@namuol namuol commented Apr 14, 2017

These failing snapshots feel backwards, like it's inverting the before and after

Yeah, the Jest output doesn't really make sense. It treats every line but the last as different, even though if you copy/paste the terminal output and do a diff after cleaning out the additional +/-s, they're identical.

@orta
Copy link
Member

@orta orta commented Apr 14, 2017

yeah, must be some invisible characters - can we strip them for the test?

@namuol
Copy link
Member Author

@namuol namuol commented Apr 14, 2017

Yup. Stripping the whitespace seemed to fix the issue. I'll make this more explicit and add a TODO to the test file.

@orta
Copy link
Member

@orta orta commented Apr 14, 2017

👍 Perfect

@namuol
Copy link
Member Author

@namuol namuol commented Apr 14, 2017

I'd wait for @alex3165's response, but otherwise does this look good?

@orta
Copy link
Member

@orta orta commented Apr 14, 2017

Yep, everything looks 👍

@orta
Copy link
Member

@orta orta commented Apr 14, 2017

will merge and release EOD tomorrow if we don't hear anything back

@orta
Copy link
Member

@orta orta commented Apr 14, 2017

also, I've invited you to the org, as a part of Moya Community Continuity - you're welcome to participate (or not, we're chill) in PRs and help out at a level that works for you.

@orta orta merged commit bcdb0ce into danger:master Apr 15, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.