What is the best way to code review a work-in-progress?

Question

I am working on a feature with a system that I am unfamiliar with. The feature is not ready, but I want to show the code to my team (who is familiar with the system) so they can give me early feedback. We are a fully remote team.

Making a pull request on GitHub so they can see the differences seems like the easiest way to do this. But then I will be creating a pull request that is not ready to be merged. This sounds dirty to me for various reasons, such as it muddies up the PRs, and someone may accidentally merge it.

I could just point them to a branch, but then they would have to find the diffs themselves, which not everyone knows how to do. They are also much less likely to review the code at all. I am hoping to let them review the code on their own time, instead of setting up yet another meeting.

Is there a standard for getting early feedback on work-in-progress features?

Answer 1

GitHub allows for PR to be in a "draft" state. Your team can see the differences, and even comment on it, but it's still obviously a work-in-progress, and cannot be merged until you click a "ready for review" button, which makes it mergeable.

I'd also say that if it's a work-in-progress, give them a clue as to what you want them to focus on, such as saying "I'm most concerned about the payment processing algorithm in SomeClass". That way they don't spend time reviewing other parts of the code that are subject to change even as they review.

See Draft Pull Requests and Convert Pull Request to Draft.

Answer 2

This comes down to the tools that you are using.

Since you mention GitHub, I'll point out that GitHub supports draft pull requests. Other tools may also have similar capabilities. The purpose of draft pull requests is exactly what you point out - to let people have a good view of the pull request yet prevent merging until it's ready.

Not all tools support this, though. Bitbucket, for example, doesn't. However, you can configure tools like Bitbucket to prevent merging if someone adds a task. For a draft pull request, a developer can open the pull request and add a task. Not assigning assignees to review can also indicate that it's a draft and make it less likely to merge.

For developers, though, I don't think that pointing them to a branch and asking them to use diff tools to compare would be a problem. This kind of functionality is built into a lot of mainstream IDEs already.

Answer 3

All code is a work in progress and every PR is a request for comment. Since our tools should be used to help us work, we first should decide how we want to work.

1. Always merge your own PR, and never merge someone else's PR.

This is your code and you're the expert at how it should be merged and how conflicts would be resolved. If it's understood that you're responsible for your own code until the point it's merged, then you won't need to worry about accidental merges. For people who merge other people's code: stop micromanaging.

2. Submitting a PR should be the one and only standard for requesting feedback.

Each reviewer has their own standards and as a team you'll have common standards. There is no reason to ask for different standards when requesting a review, regardless if the code will be merged or it's just an idea. Adopting this rule also means your team knows the role of a PR and has a direction to which to improve the process. Common additions to the PR is automated testing, syntax rules, security checks, etc.

3. You should indicate in code the intentions (if possible)

In general all PRs should be ready to be merged at the time the PR is made, that's the idea all the reviewers should have when reviewing. Whenever possible, you should make features such that it can be controlled with a feature flag. Even if you're not ready to merge, or don't have the intention, your code should be. Designing features that can be controlled with flags encourages modular designs, allows the code to be merged in small pieces, and most importantly indicates this work is a Work In Progress.

4. Submit PRs along the way

As you build a feature you'll find your PR is made up of code needed to support the feature and the code for the feature itself. Support code would be refactors required, creating new utilities, adding a new service, etc. While this is needed for your PR's main feature to work, it could also be done as its own PR. This allows you to get feedback early while also being able to merge the code, in addition you can continue to work on feature while waiting for comments on the smaller ideas. When it comes to the main feature, there should be far less feedback to even request.

5. All code is work in progress

All code merged in should be near perfect, but with the understanding that you can always make another PR. The first priority of a PR is to ensure the code won't break the application, so that should be the main purpose of the review. The second priority is to ensure the code can be maintained, like style and common practices. Anything beyond the strictest measure of correctness and strictest definition of maintenance is extra, and should be understood as such. Always feel free to make any comment, but also have the goal that the code should be merged in ASAP. But don't create TODO tickets just to get a PR merged, that's purely unethical.

6. The PR process is also a work in progress

Teams of different sizes and capabilities will have different practices. As your team grows and evolves, make sure you allow your process to grow and evolve too.