search

danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.27k stars 368 forks source link

Support ignoring failures on comments outside PR diff range #981

Open sgtcoolguy opened 4 years ago

sgtcoolguy commented 4 years ago

Hi there, Danger.JS is awesome and we use it extensively in our CI builds. Just ran into an issue where we're not getting updated comments/results on our Github PRs because of a plugin (a fork of one we use) that tries to translate eslint warnings/errors into Danger comments: https://github.com/sgtcoolguy/danger-plugin-eslint/blob/master/src/index.ts

Basically, it's possible that a user touches a file that has existing warnings from eslint, but doesn't edit the lines mentioned in the warning. The plugin will attempt to set the warning comment because the file is in the listing for the PR, but Github's API will throw an error that the "position" is invalid.

Maybe there's some way to have the plugin denote that the comment isn't "required" to be set or something? Or is there some way I could tell if the warning would have an invalid position by pre-filtering based on the diff chunks for the file? Then I could have it not do inline comments but just generally list the warning/error.

It's odd, given that it seems Danger was able to find a position here: https://github.com/danger/danger-js/blob/master/source/platforms/github/comms/issueCommenter.ts#L14 since the error came from the Github API response.

Relates to #770

fbartho commented 4 years ago

I think GitHub may have changed behavior here to be stricter — a month ago, our backend team added a linter to CI (not danger) and it erroneously (but successfully) commented on lines outside of the PR diff.

My team hasn’t seen this with danger-JS, but we fully lint clean on that project.

orta commented 4 years ago

Yeah, we're effectively trying to re-create their constraints - it's quite possible that they added new features since then (or changed their model). That code is a few years old now. It's also possible that multiline, or review comments have different constrains also.

fbartho commented 4 years ago

I don't see a big problem if we allow it to fail-silent? Maybe optionally add a warning "Some lint warnings or errors failed to post, possibly because they were on lines unchanged in this PR"?

orta commented 4 years ago

I'm not sure if it's in here or Danger Ruby, but in one of them api fails are caught and put in main comment