danger / danger-js

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

[Feature Request] Support ignoring whitespace-only changes #1420

Closed mgol closed 7 months ago

mgol commented 10 months ago

git diff supports the -w flag allowing to hide whitespace-only changes. GitHub also supports it via both the UI and via attaching a w=1 query parameter to the URL.

It'd be great if Danger's utils like danger.git.diffForFile supported this as an option. Right now, the only way to achieve such behavior seems to be triggering Git commands directly.

A use case I have is banning a certain class of modifications unless you are a privileged user - e.g. to prevent people from adding more @ts-ignore comments to TS files. I do want to allow refactoring like putting the code in an if clause without triggering these checks - I'd like to only evaluate the if line in such cases.

orta commented 10 months ago

Danger JS doesn't use the CLI for git generally, we rely on generating a diff from the API (so it can run without a local copy of the repo) but from the docs for github - I'm not sure it's possible as the API doesn't document you can use it.

Open to a CLI flag like --hide-whitespace if GiHUb does support it though, feels very reasonable to me

https://github.com/danger/danger-js/blob/5edb114d07d57292c707bba1dd72b129edd2f44f/source/platforms/github/GitHubAPI.ts#L335-L342

mgol commented 10 months ago

Thanks for the reply, I see. I don't currently see a way to achieve that via the API, unfortunately... 😕

fbartho commented 7 months ago

I’m going to close this ticket for now, but if people want to re-open it, and implement a PR (say if the API changes), then we can totally reopen it.