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

GitDSL: Diffs for added and removed files #368

Closed
bdotdub opened this issue Sep 5, 2017 · 2 comments
Closed

GitDSL: Diffs for added and removed files #368

bdotdub opened this issue Sep 5, 2017 · 2 comments

Comments

@bdotdub
Copy link
Contributor

bdotdub commented Sep 5, 2017

Calling diffForFile unexpectedly returned null for me for a file that was added. It looks like expected behavior per these lines:

const modifiedDiffs = fileDiffs.filter((diff: any) => !includes(addedDiffs, diff) && !includes(removedDiffs, diff))

const structuredDiff = modifiedDiffs.find((diff: any) => diff.from === filename || diff.to === filename)


It was initially unclear to me why diffForFile would strictly return the diffs for modified files only. I can understand why we would want this conceptually (diffs are a byproduct of a change of a file), but there is definitely utility in being able to get a diff for created and removed files.

I'd like to propose that diffForFile return a TextDiff object for added & removed files. For added files, added will be the whole file and removed would be empty, and vice versa for removed files.

It would make getting all changes in the whole PR much easier, rather than calling diff on some, and fileContents on others:

danger.git.created_files
      .concat(danger.git.modified_files)
      .concat(danger.git.deleted_files)
      .map(p => danger.git.diffForFile(p))

// vs.

danger.git.modified_files
      .map(p => danger.git.diffForFile(p))

const pr = danger.github.pr;
danger.git.created_files
      .map(p => danger.github.utils.fileContents(p, pr.head.repo.full_name, pr.head.sha));
danger.git.deleted_files
      .map(p => danger.github.utils.fileContents(p, pr.base.repo.full_name, pr.base.sha));

(The above code also isn't working for me because of #367)

This change would extend to JSONDiffForFile and JSONPatchForFile.


Barring that, the documentation should be more explicit in communicating this.

Happy to do the work but wanted to get some thoughts!

@orta
Copy link
Member

orta commented Sep 6, 2017

Yeah, I think this behaviour is a great call 👍 - I think your expectations there are what I'd imagine too

bdotdub added a commit to bdotdub/danger-js that referenced this issue Sep 7, 2017
bdotdub added a commit to bdotdub/danger-js that referenced this issue Sep 7, 2017
bdotdub added a commit to bdotdub/danger-js that referenced this issue Sep 7, 2017
bdotdub added a commit to bdotdub/danger-js that referenced this issue Sep 8, 2017
bdotdub added a commit to bdotdub/danger-js that referenced this issue Sep 8, 2017
bdotdub added a commit to bdotdub/danger-js that referenced this issue Sep 8, 2017
bdotdub added a commit to bdotdub/danger-js that referenced this issue Sep 8, 2017
@orta
Copy link
Member

orta commented Sep 16, 2017

This is in 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants