How do I find a good balance in the number and sizes of pull requests I raise?

Question

Background

I've been working with a team of 6 for the last couple of months doing enterprise software development. About half the team is very new (2-6 months). We loosely follow Kanban and manage/track work through Azure DevOps, with code reviews using pull requests.

Challenge

I'd like to get early feedback on my code changes and avoid getting bogged down in reading/understanding the existing code. So I have set a personal goal of raising at least one PR a day, which my team lead is happy with.

However, it often takes 2-3 days for a PR to go through the team's approval processes.

I've already found PRs starting to pile up, with 2-3 PRs sitting unreviewed for several days. Also, there's a bit of overhead to raise PRs on related code as I basically have to split each commit to a new branch, rebasing the commit to the original branch so on the PR only the changes for that commit are shown.

Questions

1. How do I find the right balance in the quantity and velocity of PRs to get the code reviewed and deployed? I've had devs (often the same person) alternately tell me to write smaller and more frequent PRs and to combine changes into larger, less frequent PRs.
2. Should I raise a new pull request before the previous (often related) one has been merged, in a new branch like I've been doing?
3. Should I be using a different approach for code reviews? PRs are what my team is used to, and about half the team seem to prefer async communication (slack messages, PR comments etc) over a voice or video call.
4. Should I be setting a different goal/find a different way to measure my productivity?

Any suggestions welcome. Thanks!

Answer 1

Ideally, one PR should refer to one feature and one feature should have one PR (per affected repository). That way, a reviewer can get a good overview of all the changes that were made to implement the feature. That overview is needed for the reviewer so they can tell if anything was missed in implementing the feature.

Having each PR refer to only a single feature is a good way to ensure that the PR doesn't grow too large (unless the feature itself is already very large).

If you like to have early feedback on your code, you can agree with your team that you will open a PR as soon as you want to receive feedback and that you will continuously update that PR while you are working on the feature. Then you also need to agree with the team on how to signal that you believe that you are done implementing and the "final review" can be performed.

2. Should I raise a new pull request before the previous (often related) one has been merged, in a new branch like I've been doing?

Ideally, if you start on a new feature while waiting for approval on the PR of the previous feature, you should pick a feature that does not depend on the previous feature having been completed. Then you can just branch off from the main development branch and not worry about the open PR.

If you need to start with a dependent feature, then branch off from the branch with the open PR. When creating the next PR, target either the branch you started off from, or the main development branch, based on which will give the most useful review experience.

4. Should I be setting a different goal/find a different way to measure my productivity?

IMHO, you should definitely find a different way of measuring productivity. PRs are a quality-assurance tool, not a measurement for productivity.

Answer 2

"I'd like to get early feedback on my code changes and avoid getting bogged down in reading/understanding the existing code"

It sounds like you are raising PRs you know aren't ready to be merged as a way to request help with your work.

"I've already found PRs starting to pile up"

It sounds like your teammates don't want to help you do your work. Maybe they feel you should put more work into your PRs before asking them to review.

Ideally PRs have zero issues and the PR is just a human check that you are following the rules. Tests are written and pass, work item is associated etc

Obviously there is a lot to be said about the best way to do PRs, but the way you are asking makes it sound like its a #theWorkplace problem rather than a #softwareEngineering problem.

Answer 3

(1.) How do I find the right balance ...

That's between you and your team.

It sounds like your team doesn't take PRs seriously, or at least not yours. Three days is too long. Hopefully a single thumbs-up suffices to merge a PR in your repo.

I view PR invites as a high-priority task I should attend to quickly, as it is blocking a colleague. That said, if I notice a PR is "long" I will put it off until I finish what I'm working on, and engage with it at end of day or first thing in the morning. Short PRs I will try to understand and approve right away. I approve lots of single-line PRs. Make it easy for your reviewers to approve what you send them.

(2.) Should I raise a new pull request before the previous (often related) one has been merged

No.

Having submitted PR-A, that code is now snapshotted for reviewers. Begin work on branch B, cloned from A. At some point, perhaps end-of-sprint, we anticipate that all related branches will be reviewed and merged, so that many of your feature branches will be cloned directly from develop.

Submitting PR-A says "I'm willing to merge A down to develop unchanged", and similarly for PR-B. The trouble is that B is based on as-yet unapproved edits which have not yet hit develop, and B is additional work on the A feature. Based on review you might want to edit A prior to a merge, and that will impact B. Don't waste your reviewer's time with such details. Wait for the dust to clear on A before submitting B. If need be, switch to working on an unrelated feature that can make progress in parallel. It is fine to edit a branch after submission, especially in response to review remarks. But don't burden your reviewers with a stream of edits on open PRs. If you notice that happening, it suggests that the code was not yet ready at time of submission.

(3.) Should I be using a different approach for code reviews?

Yes.

If there is something very specific you're unsure of, commit & push it and then Slack one colleague the relevant github URL and question. It should mention a commit hash and line number, e.g. https://github.com/freebsd/freebsd-src/blob/78599c32/usr.bin/grep/grep.c#L65

You might suggest that "I'm about to delete this line -- do you know of something that lacks a unit test which might break?"

The idea is to work out riskier edits prior to PR, so that approving the final PR is "easy". This is similar to conducting the business of a meeting prior to its scheduled time, so participants can efficiently rubber stamp something they already agree with and get back to their other work.

(4.) Should I be setting a different goal / find a different way to measure my productivity?

What?!?

You misunderstand what the PR process is about. Productivity is measured in terms of value delivered to the business, not in commits, pushes, SLOC, PRs, or other technical details that the business doesn't care about. Have a chat with your team lead or someone else in your management chain to clarify what matters to the business.

Answer 4

Followed methodology, although informal, is unfitted for the development activity of the team if work clutters. There are several uncertainties that to be clarified requires to question the way team collaborates. Kanban's "stop the line" concept could be used to identify possible causes and solutions. All the question from the descriptions are good questions and are to be addressed by the whole team.

There are a lot of maybes among of them being that maybe followed methodology is unfitted for the actual development activity. Code reviews could be replaced by high unit test coverage rate and static code analysis tools.

The methodology to follow was probably a team decision so it has to be the decision to change it.