4 ms·

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 commen

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.

majormajor 5y ago
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!
jcranmer 5y ago
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.
- deleted 5y ago
  [deleted]
- thombles 5y ago
  > github seems to almost go out of its way to prevent you from thinking about PRs as if they were patches Those who take care to write meaningful per-commit descriptions are actively punished by the eager collapsing. Either reviewers have to (1) spot the ellipsis (2) click on it for each individual commit, or the submitter has to copy-paste all of their commit information into the PR description.
- sdesol 5y ago
  > 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'm trying to work on solving this and I have a first iteration of it which I wouldn't mind getting feedback on. If you look at the following PR: https://oss.gitsense.com/insights/github?bt=open&q=microsoft%2Fvscode%3A144787&t=crc-activity-pom-changes&v=microsoft%2Fvscode%3A%3Agithub%3Amicrosoft%2Fvscode%3A%3A%3A%3A https://oss.gitsense.com/insights/github?bt=open&q=microsoft... you can easily identify which commit does what. For example, if you select the first two commits and filter by them, the PR tree will show you the files that were changed by those commits and if you click on the commit in the tree, you can view the diff for that commit. In the future, I want to support what you described and make it very easy to diff any revision.
eloisius 5y ago
This sounds like I’m “holding my phone wrong.”
inetknght 5y ago
> Maybe they're trying to nudge us towards smaller PRs. Many reviewers gloss over 300+ line changes in a single file. Unfortunately there are a massive number of codebases where a "simple" change can mean changing _lots_ of places.

majormajor 5y ago

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!

jcranmer 5y ago

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.

deleted 5y ago
[deleted]
thombles 5y ago
> github seems to almost go out of its way to prevent you from thinking about PRs as if they were patches Those who take care to write meaningful per-commit descriptions are actively punished by the eager collapsing. Either reviewers have to (1) spot the ellipsis (2) click on it for each individual commit, or the submitter has to copy-paste all of their commit information into the PR description.
sdesol 5y ago
> 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'm trying to work on solving this and I have a first iteration of it which I wouldn't mind getting feedback on. If you look at the following PR: https://oss.gitsense.com/insights/github?bt=open&q=microsoft%2Fvscode%3A144787&t=crc-activity-pom-changes&v=microsoft%2Fvscode%3A%3Agithub%3Amicrosoft%2Fvscode%3A%3A%3A%3A https://oss.gitsense.com/insights/github?bt=open&q=microsoft... you can easily identify which commit does what. For example, if you select the first two commits and filter by them, the PR tree will show you the files that were changed by those commits and if you click on the commit in the tree, you can view the diff for that commit. In the future, I want to support what you described and make it very easy to diff any revision.

eloisius 5y ago

This sounds like I’m “holding my phone wrong.”

inetknght 5y ago

> Maybe they're trying to nudge us towards smaller PRs. Many reviewers gloss over 300+ line changes in a single file. Unfortunately there are a massive number of codebases where a "simple" change can mean changing _lots_ of places.