33

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?

Evorlor
  • 1,440
  • 2
  • 16
  • 22
  • Does this answer your question? [code review with git-flow and github](https://softwareengineering.stackexchange.com/questions/187723/code-review-with-git-flow-and-github) – gnat Feb 10 '21 at 15:45
  • 1
    @gnat Thank you, but it does not. I am asking about reviewing code that is incomplete and not ready to be merged. That question is about ready-to-merge features. – Evorlor Feb 10 '21 at 15:50
  • 1
    Have you considered pair programming? – Didier L Feb 11 '21 at 09:47
  • 1
    Frame challenge: Don't send a group mail asking everybody to chime in. Pick one fellow team member if they will review your code. (And I'd also opt for a personal meeting instead of e-mail ping-pong. When you're new in an area, you have too many wrong assumptions and open questions to be effectively communicated in writing.) You can't have it both ways: Either you (potentially) inconvenience someone or you don't get useful feedback. – Jann Poppinga Feb 11 '21 at 13:43
  • 5
    "then they would have to find the diffs themselves, which not everyone knows how to do" ...what? It may not be the ideal tool for your specific task, but this is such a basic development tool that if technical members of your team don't know how to compare two branches in git then I would seriously be having a rethink. (Hint: In GitHub, the button right next to "Create pull request" says "Compare".) – user3067860 Feb 11 '21 at 19:12
  • 1
    @user3067860 indeed. Also there's `git diff first-branch second-branch`. Very complicated indeed. Devs need their time for more important tasks than learning a half-liner of the most important program in their career (IMHO git is more important that gcc, by a slight margin). – Vorac Jul 28 '21 at 14:04

3 Answers3

72

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.

Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
FrustratedWithFormsDesigner
  • 46,105
  • 7
  • 126
  • 176
  • 13
    Another convention is to just use `[WIP]` in the title of the PR. – phipsgabler Feb 11 '21 at 09:47
  • 10
    "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" — this feels very important, +1. If you don't do this in advance, you're likely to get feedback on things you know aren't right (not linted, lacking tests or whatever) which will be frustrating to you (since it's on your TODO list) and frustrating to them (since they put in the time for granular feedback & it's not valuable). – anotherdave Feb 11 '21 at 12:08
  • 1
    Note that this feature is only available for repos that are open source, or paid private to the enterprise. Essentially it’s a paid feature (+ for open source) – Kzqai Jun 04 '21 at 14:36
  • 2
    @Kzqai: I didn't know that feature was restricted, I guess because we're using it on open-source repos. – FrustratedWithFormsDesigner Jun 04 '21 at 15:36
9

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.

Thomas Owens
  • 79,623
  • 18
  • 192
  • 283
  • 2
    just for the record. GitLab implements MR (Merge requests) and it's basically the same thing. – Laiv Feb 10 '21 at 16:05
  • "asking them to use diff tools to compare would be a problem" Ah yes how we did it in the "good" old days. It'll probably be a good reminder of how awful that experience was and why PRs took over the world so quickly. So no please don't do that. – Voo Feb 11 '21 at 08:26
  • @Laiv Gitea also implements draft PRs. – wizzwizz4 Feb 11 '21 at 09:23
  • 1
    @Vood I have merged two forks with 6month of changes never synced, with Meld and many questions to both teams XD – Laiv Feb 11 '21 at 10:01
  • 2
    @Laiv "They were so preoccupied with whether or not they could, they didn't stop to think if they should" ;-) – Voo Feb 11 '21 at 13:33
7

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.

  1. 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.

  1. 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.

  1. 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.

  1. 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.

  1. 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.

Michael Ozeryansky
  • 220
  • 2
  • 10
  • 1
    "Always merge your own PR, and never merge someone else's PR" — many environments have decided how they want to work is by having "core" committers, so you often _can't_ merge your own PR, one of the core team must merge. This is why it's called a Pull **Request** in the first place. – Stephen P Jun 03 '21 at 17:14
  • 1
    @StephenP Pull Request is a tool terminology, and not a part of git itself. This goes along my intro: "tools should be used to help us work". And what you are describing is how the tool is instructing your work. I have worked in companies with very strict Change Control, and everyone merged their own code just fine, so I've experience the possibility. The company invested the effort to solve the problem for the employees instead of for the business. I recommend checking out Extreme Ownership and the practices taught there. – Michael Ozeryansky Jun 03 '21 at 21:09
  • I think this also needs comments that don't assume the developer is already all-seeing and fully-fledged, rather that the dev may not yet understand the codebase, so interaction and support is a better model in those cases. – Philip Oakley Jun 17 '21 at 13:22
  • @PhilipOakley I didn't intended to write this assuming any level, and actually re-reading my post I still think it would apply to any experience. Giving new devs the same freedom as a senior devs is fairly important, and there's nothing stopping a senior from helping or for the dev to ask for help. Having an open process would allow entry level engineers to focus on the code itself and not the process. – Michael Ozeryansky Jun 17 '21 at 19:04
  • @MichaelOzeryansky I think we are looking at the two sides of the same coin ;-) Freedom + Support in appropriate measure, and being careful about expectations. – Philip Oakley Jun 18 '21 at 15:37
  • @MichaelOzeryansky what if less senior devs make logical errors? What if they dont add a test? My issue is that less experience devs may not cover all the tests scenarios, fail to follow SOLID (or just have spaghetti code) and in some cases might not even do what was asked of them in the first place. Code debt is can get very expensive at times. How would you propose to deal with this type of issues? I gave a dev with 4 years of experience a small task and suggested he'd use builder pattern for data seeding, he got back to me with a code that implemented everything single pattern from GoF :D – user3402600 Jul 05 '21 at 20:33
  • @user3402600 I have a lot of opinions about being on a team, as far as code review I'll stick to my post above. (1) They can only merge when approved, do you approve? (2) The act of the PR is still asking for feedback, it's not too late. (3) Does the code itself explain why he didn't do exactly as you asked? Maybe it's better, ask him to explain how. (4) Hopefully he attempted something small first (5/6) You can always change it when it breaks. To me, that is the true moment of learning for everyone. The real question is what happens if it's fine in the end, what will you do? – Michael Ozeryansky Jul 06 '21 at 01:06
  • @MichaelOzeryansky I personally, encourage everyone to do small PRs, and with example I gave, that dev worked with me for the first time, so I had to explain better on what "small" is, which helped. However, code structuring issues is my pain point, which stops me from letting people just going free. TDD, Modular Structure, DDD and SOLID are skills that take years to master... I don't how you can get a mid dev to just hit all the points first time round unless tasks are tiny. – user3402600 Jul 06 '21 at 09:22
  • @MichaelOzeryansky I had a think about your question `what happens if it's fine in the end, what will you do` and I personally see it in black & white... There is a list of things that each PR must have for it to be merged, i.e. meets business requirements, strong test coverage and readability. With those two things PR can be refactored in the future. If something doesn't fit that criteria then it's not fine. – user3402600 Jul 06 '21 at 17:10