# Pull Request Etiquette

> Metrics matter... sometimes.

There's a 50/50 chance or so that when I walk into a new org. the pull request culture isn't healthy.  Like most things in engineering, it's difficult to quantify metrics that are of value.  Lines of code committed?  Number of commits?  They can be kind of meaningless, but I'd like to propose: pull request metrics are not.

Everyone and their mother will tell you something like "developers do not like to context switch", which makes for an interesting story here.  While I recognize that might be true for many, developers are often unaware (at best) what the consequences of not reviewing code is:  You're causing others to context switch.  
> 
> When you ignore the messages and pings about reviewing someone else's code you are forcing them to context switch, the very thing you protest.
> 

Ideally you can write you code, create a PR, and maybe do a few back-and-forths to get your code out the door, and on to the next task, right?  Or would you rather half a dozen PRs until they are ready and deal with the context switching, stale code, and conflicts?  I think most would say the former.  
> 
> That being said, to be a good team player you should be on top of PR reviews.
> 

Some work cultures are not in tune with this idea and personally I think it's the biggest mistake an engineering organization can commit.  If we think about a company as a machine then it's easy enough to visualize it as a series of gears -- once the gears get gunked up then the machine has to slow down, we have to perform maintenance, and then resume again.  A second or two of a pause probably is consequential, but up that to hours and multiply it by the size of your engineering org and it spells trouble.

Hopefully that all makes sense.  Keep the machine oiled and it's happy to keep churning.

So on to the metrics.

I found this problem to be true with at my current company and I set up [PullKeeper](https://pullkeeper.dev/).  I had never used it before, but it's free(ish) and it looked like it had what I wanted.  And guess what?  It proved my suspicions to be true.  A few people hardly review code at all, there are some who take more than a day to get to reviewing (despite how much you ping them), and some hold up PRs for days, when no one else does.  To me some of these metrics scream not a team player.  Let's look at a few:

Review count: the number of PRs they have reviewed.  Pretty simple, right?  If someone's count it far lower than others' then they aren't pulling their weight.  We don't want selfish developers!  The higher the better.

Average review time: the time it takes for someone to start reviewing code, after it's been opened.  This is a big one, too.  This should be 60 minutes or less.  It's totally fine to spend some time to tie up loose ends, finish a thought, and then review some code. If developers are taking too long to help others then they aren't being conscious of other peoples' time (or the machine).

Time since last PR: how long ago the developer opened a PR themself.  Small concise pull requests are nice.  It's not always the best indicator, but if some developers only open a PR every few days, when the average is much lower, then there might be a problem.  Maybe they can't break down stories well or maybe they are just struggling in general.

The others are a bit subjective I feel.  Number of lines reviewed, average lines changed, meh.  We are getting into typical GIT heuristics that probably don't mean much.  Maybe someone takes on more bugs than others, then their numbers would be much lower than others.  There's no need to discourage that behavior... we all hate bugs.  Maybe someone takes on bigger features, I think we should let them as well; we are all motivated by different things.

In closing, there are some stats we can take seriously. We want developers that are helpful to others.  Being selfish is like having a car that's long overdue for an oil change -- it's going to get gunked up.  Forget lines of code changed and how often people commit, let's take metrics that demonstrate they are a good team player seriously and keep the machine well oiled.

Read online: https://davidoelfke.dev/blog/pull-request-etiquette-3ABhqKNno0g6GlbHYclRm0