Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Pull Request File Tree Feedback

github.com

Post reply on HN
Print view View original

21–30 of 77 posts

Re: Pull Request File Tree Feedback

#21

Post by inetknght »

eloisius wrote: post #17

I’d settle for them making PRs as useful as they were in 2015, before they messed up some of the most basic functionality: showing the diff, and showing review comments. They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. Then, once you submit a review…

> They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something.

This. Every PR I have to do ctrl-F "load diff" and then immediately click on _all_ of the diffs. It's !@#$ing annoying.

I've also lost comments when the comment is part of a review and pushed to the PR after the the PR has been merged by someone else.

Re: Pull Request File Tree Feedback

#22

Post by 88913527 »

eloisius wrote: post #17

I’d settle for them making PRs as useful as they were in 2015, before they messed up some of the most basic functionality: showing the diff, and showing review comments. They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. Then, once you submit a review…

Maybe they're trying to nudge us towards smaller PRs. Many reviewers gloss over 300+ line changes in a single file. Approved with a "LGTM" and no further comment, but perhaps that's more a cultural issue with the team than anything.

Re: Pull Request File Tree Feedback

#23

Post by corndoge »

inetknght wrote: post #21
eloisius wrote: post #17

I’d settle for them making PRs as useful as they were in 2015, before they messed up some of the most basic functionality: showing the diff, and showing review comments. They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. Then, once you submit a review…

> They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. This. Every PR I have to do ctrl-F "load diff" and then immediately click on _all_ of the diffs. It's !@#$ing annoying. I've also lost comments when the comment is part of a review and pushed to the…

Might be worth making a bounty for it in refined github[0], similar things have been implemented in the past[1]

[0] https://github.com/refined-github/refined-github

[1] https://github.com/refined-github/refined-github/issues/2151

Re: Pull Request File Tree Feedback

#24

Post by 88913527 »

ydnaclementine wrote: post #4

Not sure if this is well known, but press period `.` when viewing a PR, repo, or file and github will send you to a in-browser visual code editor. Able to make commits in there too, perfect for [nit] comments

I often use `t` to quickly search for a file by name. It's got reasonably good matching: querying for "foo.svg" will show a match for "foobarbaz.svg".

Re: Pull Request File Tree Feedback

#25

Post by majormajor »

88913527 wrote: post #22
eloisius wrote: post #17

I’d settle for them making PRs as useful as they were in 2015, before they messed up some of the most basic functionality: showing the diff, and showing review comments. They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. Then, once you submit a review…

Maybe they're trying to nudge us towards smaller PRs. Many reviewers gloss over 300+ line changes in a single file. Approved with a "LGTM" and no further comment, but perhaps that's more a cultural issue with the team than anything.

While smaller PRs are better when practical/possible, the current Github design doesn't encourage them at all. If anything, it makes it too easy to submit giant ones - a bunch of the changes may get hidden anyway, making it look less intimidating than it should be!

Re: Pull Request File Tree Feedback

#27

Post by jcranmer »

88913527 wrote: post #22
eloisius wrote: post #17

I’d settle for them making PRs as useful as they were in 2015, before they messed up some of the most basic functionality: showing the diff, and showing review comments. They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. Then, once you submit a review…

Maybe they're trying to nudge us towards smaller PRs. Many reviewers gloss over 300+ line changes in a single file. Approved with a "LGTM" and no further comment, but perhaps that's more a cultural issue with the team than anything.

Perhaps, but github's PR UI goes to utterly atrocious if you try to do something along the lines of "a series of small atomic changes". In my recent reviewing experience, anything involving multiple commits in a single PR results in the diff more or less lying to you: you get diffs of something, but it's not clear what, and even less clear how to get what I want to actually diff.

I think the evidence is pretty clear that the most effective development process for large codebases is to look at changes as effectively a series of patches applied to head-of-trunk (and those patches may evolve during review)... and github seems to almost go out of its way to prevent you from thinking about PRs as if they were patches.

Re: Pull Request File Tree Feedback

#29

Post by [deleted] »

jcranmer wrote: post #27
88913527 wrote: post #22

Earlier quoted context omitted.

Maybe they're trying to nudge us towards smaller PRs. Many reviewers gloss over 300+ line changes in a single file. Approved with a "LGTM" and no further comment, but perhaps that's more a cultural issue with the team than anything.

Perhaps, but github's PR UI goes to utterly atrocious if you try to do something along the lines of "a series of small atomic changes". In my recent reviewing experience, anything involving multiple commits in a single PR results in the diff more or less lying to you: you get diffs of something , but it's not clear what, and even less clear how to get what I want to actually diff. I think the evidence is pretty clear…

[deleted]

Re: Pull Request File Tree Feedback

#30

Post by andrewf »

eloisius wrote: post #17

I’d settle for them making PRs as useful as they were in 2015, before they messed up some of the most basic functionality: showing the diff, and showing review comments. They hide big diffs behind a “load more” link, and as a result people often fail to code review the most substantial part of a change because they scan right past it, thinking it’s a removed file or binary or something. Then, once you submit a review…

"Our metrics tell us that with these UI changes, people are approving more PRs and it's taking them less time!", probably.
Post reply on HN