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

Run Danger-prose over diffs, not modified files? #13

Open
suchow opened this issue Aug 29, 2016 · 1 comment
Open

Run Danger-prose over diffs, not modified files? #13

suchow opened this issue Aug 29, 2016 · 1 comment

Comments

@suchow
Copy link
Contributor

suchow commented Aug 29, 2016

It seems that this plugin runs over the entire modified file, including text that was merged in previous commits but which is not modified in the PR under consideration. See, for example, amperser/proselint#609 (comment). Though these suggestions are valid, they are not not relevant to the PR and thus should not be included.

Fixing this issue is complicated because it requires determining which errors raised can be attributed to the PR. One possible solution is to run the prose linters twice, once over the proposed text and once over the original text. Any error that is flagged in the former that did not appear in the latter can be attributed to the proposed change. A second possible solution is to attribute to the PR any error that appears on a modified line. This will falsely flag existing errors that are adjacent to changes, but is a step in the right direction.

@orta
Copy link
Collaborator

orta commented Aug 29, 2016

I have an example of doing this kind of analysis in the Dangerfile for Danger.Systems

diff = git.diff_for_file("plugins.json")
if diff
  current_plugins = JSON.parse(diff.blob(:dst).contents)
  pr_plugins = JSON.parse(diff.blob(:src).contents)

Where plugins.json may have changed from ["thing"] to ["thing", "other"] and then I can pull out "other". This could be used to get the before and after, and do the analysis on that.

I'm not sure how that handles new files though, but I think this is a good idea overall 👍

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