
Give some love to your PR - elleflorio
https://www.florio.dev/20200712-pr-love/
======
2bitencryption
"Keep your PRs as small as possible"

In my org we struggle with this because of two combating philosophies -- the
first one is the obvious one, "Keep it short and sweet so it's easy to
understand and review."

The other side is, "Please stop creating PRs and checking in dead code that is
never executed until a promised future PR unlocks it."

My particular product is very complex (read: messy) and minor changes involve
lots of work to maintain back-compat.

When we tried the "small PRs", we gained easier reviews... but ended up with
so many changes that require a future PR to complete the work, and the future
PR never comes for some reason or another. Another problem for reviewers is
"Even with the PR description filled out, I don't see the value of this PR
unless I see all of it together."

If you're reading that you probably scoff, "Just fix your process" or "Tackle
your technical debt so your PRs are simpler!" You're not wrong, but if only it
were something that could be done in a week and not years :)

~~~
bendiksolheim
You are alone with this problem. Finding the right size for a PR can be really
hard. Too big, and no one really wants to review it because it is too time
consuming. Too short, and it fails to show the bigger picture. We struggle
with exactly the same thing, with a bias towards too large PRs.

I am not even sure I can always agree with myself here. I tend to favor small
PRs myself, but I also really dislike PRs that are only part of the solution.
PRs should not only be about critiquing the actual code lines, it should also
evaluate and discuss the overall architecture, security aspects, performance,
readability, tests and probably other things as well. This is difficult to
achieve with small PRs.

The solution is never to "just fix your process", in my opinion. The correct
process depends on the people involved, the culture in the team and
organization, the importance of the service in question, among other things.
Finding the correct process is a never ending task :)

------
acrooks
My usual strategy is to write PRs for the end-to-end functionality of a
deliverable (meaning they can get quite large), but with many small commits
that can be stepped through during the review process.

When I'm ready to submit the PR, I rewrite the commit history so that each
commit is an independent and easily consumable chunk of code - many of them
might be isolated additions of helper methods, etc. One commit may be a
database migration, another the REST API to talk to the database, each one
having full tests for that discrete chunk of code.

The result of this is a PR possibly with 30+ commits for a large feature where
each commit could be independently merged to master that, when stepping
through one-at-a-time, tells a logical story of how every commit combines to
deliver the full feature. Each commit message also provides a detailed
explanation of what I've intended to do and why.

This means that when I'm near delivery for a feature I have a lot of
administration work to do, but I think that the small, discrete, and clearly
explained commits provide great value when trying to reason about the codebase
down the line.

~~~
paledot
All of this, although 30 commits is getting a bit extreme. If you can write 30
commits, each with its own tests, your PR could almost certainly be smaller
while still delivering value (not dead code). But I regularly end up with 10
commits in a PR.

One major benefit is bisecting. I often don't find I've broken some automated
test until I push my "finished" code for review, and incremental changes, each
of which independently pass all tests, can be easily bisected to discover the
problematic change.

------
noname120
I keep seeing this “keep it small” mantra everywhere. There is a good reason
people do large pull requests instead of multiple dependent pull requests. The
reason is that current UIs are atrocious. For example GitHub plain doesn't
support them which makes it an absolute nightmare to review them (irrelevant
changes[1]).

Additionally, syncing the different branches with Git is a pain, especially
when you have several levels of dependencies.

[1] I'm well aware of the fact that you can set the parent branch to something
else than master. But in that case, you'll have to merge the parent branch
_after_ the branches which depend on it. This completely defeats the purpose
of splitting pull requests.

------
chdsbd
With regards to commits, I think it’s better to _not_ worry about making them
nice. I think time is better spent making the PR description clear.

Make as many commits as you want, just “Squash and merge” at the end using
your pull request body for the commit message.

~~~
paledot
Please no. Squash and merge means that when someone (maybe you) comes across a
strange-looking line of code in 18 months and uses git blame to figure out why
it was added, they'll be presented with a 1500-line wall of text with no
context.

Commits are a tool. Learn to use them.

