3 ms·

This is my way as well, but in part it's because we use gerrit instead of git(hub|lab) which explicitly allows for this workflow of separately reviewed patches

This is my way as well, but in part it's because we use gerrit instead of git(hub|lab) which explicitly allows for this workflow of separately reviewed patches that build atop each other. I'm curious, if you are using git(hub|lab), how does it work with their workflow?

alexeldeib 5y ago
It's okay on github, if somewhat implicit. You can click the "commits" tab of a PR, and then clicking any individual commit brings up the usual github diff view (like you'd see for the whole PR). It's not immediately obvious but you can leave comments in that view, unlike the other view for a single commit on branch history. But I find most people on github end up reviewing the whole PR as one unless we all are on the same page ahead of time. So despite supporting the functionality it's not all that highly used compared to, say, the way individual patches in a set might be reviewed on the Linux mailing lists during later revisions. I feel like on github it's more common to see asks to break up PRs (maybe just as a result of the projects I've seen? both approaches have valid usecases imo, and there are good reasons to break up PRs which can be done in single commits anyway). I still feel it's good hygiene even if reviewers don't look :)
- mnahkies 5y ago
  I used to use kiln from fogcreek and found that it's code review feature was much better suited for commit by commit style reviewing than either GitHub/gitlab. On GitHub it tends to push you towards smaller and smaller PRs, which is generally good, however I kinda miss the "full context" of having a larger change on a single PR with well thought out commits
acidbaseextract 5y ago
It seems like Phabricator supports this type of workflow, and I really wish there was wider popularity so I could push for it at work. The lab|hub style PR ends up with people falling into giant PRs that are difficult to review.

alexeldeib 5y ago

It's okay on github, if somewhat implicit. You can click the "commits" tab of a PR, and then clicking any individual commit brings up the usual github diff view (like you'd see for the whole PR). It's not immediately obvious but you can leave comments in that view, unlike the other view for a single commit on branch history. But I find most people on github end up reviewing the whole PR as one unless we all are on the same page ahead of time. So despite supporting the functionality it's not all that highly used compared to, say, the way individual patches in a set might be reviewed on the Linux mailing lists during later revisions. I feel like on github it's more common to see asks to break up PRs (maybe just as a result of the projects I've seen? both approaches have valid usecases imo, and there are good reasons to break up PRs which can be done in single commits anyway). I still feel it's good hygiene even if reviewers don't look :)

mnahkies 5y ago
I used to use kiln from fogcreek and found that it's code review feature was much better suited for commit by commit style reviewing than either GitHub/gitlab. On GitHub it tends to push you towards smaller and smaller PRs, which is generally good, however I kinda miss the "full context" of having a larger change on a single PR with well thought out commits

acidbaseextract 5y ago

It seems like Phabricator supports this type of workflow, and I really wish there was wider popularity so I could push for it at work. The lab|hub style PR ends up with people falling into giant PRs that are difficult to review.