10:56 a.m.
I think we all agree that PRs have many merits: it's almost pointless
to try highlighting them as we're all experienced developers, and I
guess we all have some horror stories from back in the dark ages when
we couldn't use them.
When we moved to git very few of us had some experience with it, but
those who had did propose a solid workflow; we're using this process
now since approximately 2 years, and while I think it's been quite
good, it's time to apply some Retrospective.
I won't annoy you with the long list of benefits, far more interesting
to discuss what I'm considering - in my experience - a strong
drawback:
### Delays ###
Pull requests introduce some delay; sometimes it's short and
reasonable, sometimes it's huge and well needed, but sometimes it gets
very long without providing a significant value to the project.
Actually, since these delays can cost us significant time, there is a
large opportunity cost and not all of us are always aware of what you
might be blocking.
# Context Switch #
Now even if it introduces a strong delay, often it's not a big problem
as we can "context switch" to other subjects or even other projects,
but in practice I'm struggling a lot by this context switch and -
assuming I'm not the only one hit by this - I think we should avoid it
as much as possible: context switch is not just energy draining, it's
also demotivating and has a direct impact on the code quality. That's
huge: code quality is the core of what we're trying to improve with
the pull requests process, but in practice it's achieving the
opposite!
# Real tangible delays #
In some cases, a failed review doesn't just force a context switch but
brings to failed deliveries, or unability to work on the next task. Ok
this is not common, but it happens.
# Conflicts #
This is not a soft-problem nor too hard to handle, but of course the
more we keep parallel branches the more we have conflicts to resolve.
Some of you pointed out it's no big deal but I've often volunteered
for some hard ones... I guess I should change that strategy so you can
better feel the pain ;-)
### Counter-measures ###
I don't think it's worth cancelling the PRs process as there are great
benefits, but it's worth exploring how to minimize the undesired
side-effects.
I also don't think we can nail down precise "rules" .. I will just ask
to use common sense and try hard to merge pulls quickly.
To add some specific directions: I noticed frequently some good points
are made on a PR which are not strictly a blocker to the pull itself.
Of course if the tests fail or if it introduces other blockers which
prevent other developers to continue on their work, it should not be
merged.. but in most other cases we should try to ask for "follow up"
improvements rather than "fix now".
If we can create follow-up improvements, we can easily prioritize
them. Decoupling the priority of further improvements from the main
goal of the pull request is important to make sure that we're spending
time on the highest value instead of entertaining ourselves with
design puzzlers.. I know the feeling, we all are scientists and enjoy
some nitty gritty polishing but often there's no real gain from it,
not even long term readability as I'd rather hope our code to evolve
faster.. we're not polishing a statue of immutable stone.
Also, a follow-up improvement can easily get reassigned to someone
else; for example often the contributor pointing out a possible
improvement has a better idea of the change he would like to see
applied, so he could "just code it" and propose it as a change.
Finally, we could try to *reject* PRs more often. If you think there
is a blocking problem with the pull, today we would usually ask for
improvements and wait for the puller to fix, we could as well make it
very clear when we think that it's a blocker problem by rejecting it..
I suspect that could have some good effects:
- no push-force on the same pull hiding previous dicussions
- potentially a different reviewer on the next attempt (we had cases
on pulls gettting stuck because the single reviewer was away!)
- no long lists of dormant pull requests
I think this would also encourage people to be quick in merging if
there is *any* open pull request.
The problem here is to not take it as an offence, but I think if we
were all doing it more regularly, - if it becomes a normal process
rather than an exception - then there would be no problem with it.
Possibly we would need to explain that nicely to newcomer
contributors!
# Avoid pull requests for previews
This is becoming more frequent but I came to dislike pulls which stay
open for long (days). I'd prefer to keep the queue empty and try some
alternatives, like sending pull requests to specific developer's
rather than to the main project. This way you could "invite" specific
devs, and they could close the pulls to signal they consider the
review task complete.. or explore something like that.
# Avoid showing off scanner capabilities :-)
I've been very guilty myself on this one; I remember occasionally to
have attacked a PR with a sense of "let's see how many nitty gritty
details I can find", then effectively pointing out many typos and
small inaccuracies but possibly failing to evaluate the grand plan. I
hope I've learned to "let go" a bit.. for these cases I think it's of
course great to comment on github but use good judgment to see if it's
worth delaying the merge. Often I find myself applying minor fixed
myself, some other times, I expect follow up polishing pulls, or we
open new issues.
# When to delay?
Consider that a sequence of PRs will show up in history as a sequence
of commits, and it's going to be impossible to figure out from the
master history if the changes are caused by a single PR or by multiple
PRs.
Consider also that when asking a PR improvement, we often stack
additional commits...
I really prefer if we all could "fixup" the wrong commits, but in case
there is no need for it, you could as well merge and ask for further
improvements.. could be a good rule of thumb?
Let's fight our love for perfection and bring home some *useful* stuff
ASAP. (Hint: renaming internal classes is not in this category)
-- Sanne