On Tuesday, 19 July 2016 at 06:29:05 UTC, Jacob Carlborg wrote:
On 2016-07-19 00:30, Walter Bright wrote:
https://medium.com/@kurtisnusbaum/large-diffs-are-hurting-your-ability-to-ship-e0b2b41e8acf#.h3eo1yvqv
I've been advocating for a while now that PRs should be small,
incremental, encapsulated and focused. This has not been
without
controversy. I hope the referenced article is a bit more
eloquent and
convincing than I have been.
I fully agree, the problem is if unfinished features are merged
to master, which has happened quite a lot in D. Have you read
the solution, linked at the bottom? [1]. As far as I can
remember, I have not seen this used in the D projects at all.
[1]
http://graysonkoonce.com/stacked-pull-requests-keeping-github-diffs-small/
Requiring that all contributors do this would kill D development.
This method strikes me as nothing but a very ugly work-around for
GitHub not having good facilities of reviewing PRs
commit-by-commit.
There is another, much simpler and IMO better way:
1. Contributors: Split the change into commits. Each commit
should represent an atomic change, and all commits should compile
and pass tests. (See also Linux kernel code guidelines.)
2. Reviewers: For multi-commit PRs, ALWAYS look at the changes
one commit at a time. Read the commit messages. Don't even think
of clicking on the "Diff" tab, all this does is cause threads
like these to appear. (Seriously people, why is this an issue?? I
think that these threads will never stop until someone hacks the
"Diff" tab out of Walter's GitHub UI.)
3. Contributors: Don't rebase your PR until it's ready to merge!
Rebasing will a) erase comments left on individual commits b)
make it difficult for reviewers to see new changes since their
last review.
4. Reviewers: DO use the new GitHub feature which shows a diff of
changes since your last visit. DON'T ask contributors to rebase
their PRs until it's ready to merge.
5. Contributors: Once the change is approved, optionally rebase
the PR and fold the fixup changes into whatever commits they
belong in.
This has the advantages that:
1. Until the final rebase, you can track the development history
of the PR. All feedback and changes in response to it will still
be there.
2. It's much easier to review a newer version of a PR, as you can
get a diff from the last version you reviewed.
3. The total time to merge the entire change set is going to be
much smaller, because there will be one review per PR instead of
one review per commit. From personal experience I can affirm that
when a change implemented in a few hours or days drags on its
review for a few months (or years!) is very fatiguing.
Obviously there are some advantages to splitting changes into
multiple PRs, such as better UI on GitHub and letting the
auto-tester ensure that nothing breaks each step along the way.
However, especially considering how long it already takes for
even trivial PRs to get merged, I think that a proposal to split
changes into multiple PRs would all-in-all be worse for D
development.
