The Pull Request

General Developer Forum General Dev Chat
1

When you are under pressure to deliver you ideally want your Pull Request to be reviewed, approved and merged as quick as possible. So does everyone else! I’ve seen a Pull Request go through a review for over 52 days! An extreme example, and most of the debate was petty.
But where do you draw the line? Does the wrong variable name stop you from approving someone’s work? Or, would you ask for a complete redesign of a submitted solution?

I would like to open a discussion around the Pull Request and share our experiences and practices.
Here’s some questions that I have pondered many a time:

  • How do you review Pull Requests ?
  • When do you review Pull Requests ?
  • How long are you willing to wait for a review / approval?
  • How long do you spend reviewing a Pull Request?
  • How many Pull Requests would you review in a day ?
  • What is the best size (number of modifications/ additions) for a Pull Request ?
  • How frequently do you submit Pull Requests? (daily / weekly / monthly)?
  • How do you feel emotionally when someone suggests changes?
  • How do you feel emotionally when you suggest changes?
3 Likes
2

Corresponding tweet for this thread:

Share link for this tweet.

3

I would try not to take anything personally (so long as the feedback wasn’t rude). Just keep in mind that often personal taste, direction and experience are factors, and if a change is suggested it just means they are looking for something different, which may be based on their taste or experience.

For people suggesting changes I would say please just be kind :blush:

2 Likes
4
  • How do you review Pull Requests?
    • On GitHub
  • When do you review Pull Requests?
    • When I have time after I get assigned as reviewer or pinged to review
  • How long are you willing to wait for a review / approval?
    • As long as it takes. We do not merge unreviewed or unapproved PRs, even for production critical code.
  • How long do you spend reviewing a Pull Request?
    • Depends on the PR, but generally not more than 15 minutes. If the PR is too big, we tend to mark our review as “Too much code, please split into smaller more reviewable chunks”.
  • How many Pull Requests would you review in a day?
    • Between 0 and 10. It depends on the day and what project the other developers are working on.
  • What is the best size (number of modifications/ additions) for a Pull Request?
    • I don’t think there is a “best size” or a “one size fits all” answer to this. If you’ve made the same change in a thousand files, I’ll still count it as one change. Make the changes relevant to the problem you’re trying to solve.
  • How frequently do you submit Pull Requests? (daily / weekly / monthly)?
    • Again, it depends on what I’m working on. I would say around 5-10 per week on an average week.
  • How do you feel emotionally when someone suggests changes?
    • I feel happy, because it means that they care enough to actually check out what I’ve been doing. If I agree, I’ll give a :+1: and immediately fix it. If I don’t, we can have a discussion. Sometimes that discussion happens off of GitHub (Slack, Zoom, in person) or it can happen in the review. Once we come to an agreement, we’ll post the agreed upon changes and I’ll do that.
  • How do you feel emotionally when you suggest changes?
    • If the change is “you misspelled this” or “Please follow the style guide” I get annoyed, because it feel like the other person just don’t care. When I suggest changes other than that I tend to back it up with “previously we’ve done this in file X on line Y” or “We discussed here LINK that we would do it this way” or even “I would have done it like THIS, but if you feel like we should do it your way going forward, we can do that”.
2 Likes
5

cheers @ohm ! that’s all pretty much aligned with how we work as well.

I like the idea of splitting a large PR though. We don’t see many but it could be an option if the case arises.

We also mandate a min of 2 approvals before a merge and on shared components maybe even an approve from each team. However, we do experience a lag time for feedback from other teams and at times I have waited for 2 days for an initial review before pinging. I personally don’t like to ping because everyone is busy with their own work. But sometimes it’s necessary.

2 Likes
6

“Lucky, luck, you!” With my current client and his crew, it’s routinely a week before anything is even looked at, let alone any feedback given. With the guy I need to collaborate with for most of the stuff I’ve been doing lately, it’s more like a month minimum, maybe never. And then the client boss gets pissed at me, for not having my stuff merged into the default branch in time for the demo with his client (that he never told me when it was gonna happen, except maybe that it was postponed from just after my prior last-minute death-march which is why that happened in the first place).

2 Likes
7

Hi @davearonson - that is chaotic, counterproductive and frustrating. Your client has management, communication and collaboration issues.
What Project Management process are they using? How are the delivery expectations set?
When you say “last-minute death-march”, did you leave in the end?

2 Likes
8

How do you review Pull Requests?

Same as @ohm, on GitHub, but I want to go beyond the technical answer.

When I review PRs I’m very deliberate about my choice of words. I never write “you should do X” but I opt for wordings along “from where I’m standing it might be better to do X because of reason Y”.

I always explain the reasoning behind my suggestions and make liberal use of GitHub’s “suggestion” feature to ease integrating my suggestions for the author.

I’ve actually did a lightning talk on the topic a few years back, you can see it here (here is a great article on the topic which I also reference in the talk).

When do you review Pull Requests?

Usually in the morning or directly after lunch. I try to avoid interrupting my focus to do code reviews so I do them “when I have time”.

How long are you willing to wait for a review / approval?

As long as necessary. Usually they’re done on the same day, sometimes I need to wait another day. If it takes longer than that I tend to re-request the review as a more gentle version of pinging people on Slack.

How long do you spend reviewing a Pull Request?

Anything between 5-10 minutes for short PRs to up to an hour for bigger/complex PRs.

I really go through the code and try to understand the flow of the program which takes it’s time but also lets me spot things a more superficial code review would miss.

How many Pull Requests would you review in a day?

On busy days I’m reviewing around 3-4, haven’t had to review more than that so I can’t comment on what my “upper limit” would be.

What is the best size (number of modifications/ additions) for a Pull Request?

It’s great if the PR can stick below 100 changed lines that’s great because it eases the burden to understand the change. Sometimes that’s not possible but I think it’s a good rule of thumb to try to keep PRs small.

I struggle with this myself from time to time.

How frequently do you submit Pull Requests? (daily / weekly / monthly)?

A few times per week. Not necessarily daily.

How do you feel emotionally when someone suggests changes?

Depends on how the suggestions are phrased.

See my section above on how I do code reviews. :yum:

How do you feel emotionally when you suggest changes?

As said before, I always try to stay compassionate when reviewing and try to put myself into the author’s shoes. From the feedback I’m getting from colleagues I think I’m doing quite a good job at this.

4 Likes
9

hey @wolf4earth 100% agree with the wording !
As I was enthusiastically replying and listing the phrases I use in a PR review, I stopped writing and watched your lightening talk.

:clap: :clap:

I did actually clap from my sofa when you finished - :grinning:
And I learned something : Asking Judgemental Questions

Here’s the list I was proudly writing and thinking “I’m such an understanding guy …”:

let’s rename this …
what do you think about … ?
can we … ?

All Judgemental Questions as it turns out. So I’m not going to use them anymore. I’ll have a look at the Compassionate Code website and read the article you link on Judgemental Questions slide, Unlearning toxic behaviours in a code review culture.
It’s time to learn me some compassion :smiley:

1 Like
10

After reading the article Unlearning toxic behaviors in a code review culture I see now that at least one of my phrases is actually ok.

What do you think about … ?

As long as I demonstrate a benefit.
Lots’ s of good stuff to think about and take onboard in that article.

1 Like
11

Me too Finner!

Brilliant talk Sascha! I posted it in a dedicated thread Being Humane in Tech - Sascha Wolf and pinned it in our diversity portal as well :+1:t2:

12

Here’s a free book on Code Reviewing by Trisha Gee from JetBrains

And one of her presentations

1 Like
13

Here’s another scenario :slight_smile:

You encounter this code in a code review on a backend service:

if (dataTableName.equals("abcd-table")) {
    sortBy = "field_A";
}

return sortBy;

The if statement is the code been introduced so a new piece of logic is being added here.
You need to get this into the development environment ASAP because the UI team is waiting for this.
You know that a proper generic solution will take a day or 2 more.
I see the following options (the first is my preference).

  • go ahead accept the change but then start to work on the generic change right away
  • decline the PR and build the generic solution and resubmit
  • accept PR and add tech debt to be planned for sometime in the future

Are there other options I might not have considered?

1 Like
14

Why do the UI team need this ASAP? Their project or issue should have been better scoped. This is not the developers problem, more so the UI team or their manager.

I would reject these changes - or at least accept it and add a tech debt issue to our backlog.

1 Like
15

hi @ohm !
I had originally used a different word than team, but the bot on this platform slapped my hand for using incorrect language. :face_with_raised_eyebrow:
In this case the UI isn’t a separate team, if it were I would agree with you. The team in this case is full-stack , from database to UI, which implies the feature is to be finished on the current sprint.
Maybe you are right, a reject is a clear signal that this piece of work is bigger than originally thought and it will need more time and will not be finished in this sprint. The Product can decide.

1 Like
16

What was the original sentence Finner? I just want to check to make sure it didn’t make a mistake :upside_down_face: (thank you for editing it tho! :heart:)