Hacker News new | ask | show | jobs
by akx 1120 days ago
The PR isn't very large, diff-wise, but IMO it isn't well made, which makes it hard to review.

Half of the commits are merges from other fork branches into the contributor's master, and the PR name and description doesn't mirror that in the least.

Then (eyeballing) 90% of the diff is whitespace changes, which would be fine in its own PR ("Formatting changes") because it's easy to eyeball that it's just that, but when you mix it with other changes, it's hard again.
1 comments
seedie 1120 days ago
FWIW, whitespace changes can easily be filtered out in Github's PR view.
link
robertlagrant 1119 days ago
It's a mistake to filter out whitespace changes on Python diffs. These days it's best use `Black --check` or similar in CI to make sure they've matched your whitespace settings, to minimise these changes.
link
akx 1120 days ago
Sure. The link above is in "hide whitespace" mode (`?w=1`), and there are still visible whitespace changes (new or removed whitespace lines).
link
dingledork69 1120 days ago
Like 3 of them. Is that problematic enough that you cannot review the rest of the code? My eye just slides over them.
link
DarkPlayer 1119 days ago
I am working on a GitHub pull request viewer that displays changes using a semantic diff, and therefore has some more advanced whitespace handling behavior than just ignoring leading or trailing whitespace. I tried it with this PR:

https://app.semanticdiff.com/django-money/django-money/pull/...

It doesn't make a huge difference, but it filters out changes like the added line break in "if value: value = str(value)" nicely. I haven't announced the project yet, but maybe someone will find it useful :-)

link
mindB 1120 days ago
I don't think this was the case in 2016 though.
link
wes-k 1120 days ago
They added it to the UI in 2018 but you could do it via URL since 2011.

> A diff view with reduced white space has been available since 2011 by adding ?w=1 to the URL.

https://github.blog/2018-05-01-ignore-white-space-in-code-re...

link
seba_dos1 1120 days ago
Also, Git had it since forever, and since GitHub's diff view isn't particularly convenient for browsing multi-commit PRs you usually review the changes using Git anyway.

That said, I'd ask the contributor to tidy up the branch first. It's kinda disrespecting to ask others to review branches in such state.

link