Are pull requests the place for training juniors

Question

We have a concept that all code in a Pull Request into master should be production ready. This makes sense and is a fair statement in my opinion.

The idea here being that once you create the PR, you are stating that you would have put this into master, but would like some reviewers to simply 'concur' with you and spot anything you happen to miss.

As we are only human, we make mistakes and hope that other reviewers might find items that unit tests could not find - spelling mistakes, incorrect javadocs etc.

BUT, is a Pull Request the place where we should be providing some level of assistance/training to developers and if so, to what level?

Each time you push new changes, reviewers have to re review your changes, which takes from their development time and causes re reviewing changes.

So, how much training is expected, should be allowed, in Pull Requests? Part of me feels it varies from juniors to seniors. However I also feel that it should not be the place for finding vast amounts of issues - even for juniors.

Is anyone else struggling with trying to get developers to achieve the goal of "My pull request ought to be production ready"?

Answer 1

No. Pull requests are not the place for training. Your team has the right idea. A PR means "I think it's good to go. Can I get a 2nd set of eyes on this in case I missed something. I'm human after all." And I suspect that's exactly what your apprentice is doing. They honestly think it's good to go.

Here's an extreme idea (pun intended). Pair Program with the apprentice who's giving you the heartburn. As a more senior team member, it's your job to lift them up and get them to productive.

Answer 2

If code that violates core design principles or standards of the team makes it to the pull request stage, then it should definitely be addressed there. And code reviews can be a good means of communicating the team's standards and design practices.

But is it the best place? Here's some reasons why I would say not:

1. If it takes several code review iterations to address a fundamental design flaw or large-scale issue, then there will be a strong temptation to overlook the more minor issues mentioned above, due to deadlines or general exhaustion with the review. Ideally, the team would be resilient enough to resist this temptation, but it's probably best not to create a situation that would lead to it.
2. Receiving a code review with a large number of comments isn't a great experience for a junior / new hire developer. Yes, responding to feedback is a skill they will need to develop, but it's also true that developers aren't always skilled at tactfully responding to code they don't like.

Pair programming and design reviews are preferable venues for larger scale feedback.

That said, it would be even worse to let the code go through for fear that addressing it during code reviews is "wrong," and there may be some circumstances (e.g. remote teams) where this is the best you can do. In that case, address the problems in the code review, and also address the problems in your development process that it got this far.

While finding the problem during a pull request might not be ideal, it is certainly better than in testing or in a production problem.

Answer 3

I would phrase it more as pull requests shouldn't be the only place you train people. Also, junior developers aren't the only ones who may need some training in a pull request. Contractors, open source contributors, devs from other departments who are unfamiliar with local code or standards, and even veteran programmers need occasional reminders when they get complacent.

There is very little cost to holding a pull request open for a while. Give it as much or as little feedback as you like, by as many people as you like, then leave it alone until the authors notify you again that they think it is ready for merging. A pull request is a great central tool for communicating about proposed code changes, whether they are fully-ready or need a lot of work.

Some pull requests reviews turn out as little more than a rubber stamp, and some that are external submissions to a team might take a month of back and forth. The first kind is nice when you can get it, but that doesn't mean the second kind isn't valuable. Trying to get people to submit perfect pull requests all the time is just going to be frustrating for everyone.

Answer 4

I've always felt that one of the hallmarks of a good lead is someone who provides the additional training as needed during each development cycle. For me, that means I'm not only coding myself, or reviewing code, but sitting with more junior developers, pair programming with them to help them avoid the kind of land mines I've stepped on.

Mainly, I have no illusions that our primary goal is educational - it's not. Whether you're a senior, a lead, or a junior developer, the goal isn't your edification. The goal is always to deliver quality code to the customer. Preferably on time, on budget, doing what they want. I do recognize, however, that it's impossible for me to get all the work done myself, so it's incumbent on me as a lead to help everyone help the team succeed. And that means recognizing training opportunities when they appear in nature.

So, to your question about whether pull requests are the place for training juniors, I would have to say that it's not uncommon for teachable moments to arise during these. Hey, you're going to have to deal with your first merge conflict, let's go over that after the review. Oh look, you didn't include any unit tests for your DAO, we'll postpone your review until after we've had a chance to cover those new methods. Why did you think it would be better to use double primitives in this financial calculation than BigDecimals? Yeah, that's not really uncommon.

So, while I would say that it certainly can happen, but it's clearly not the main goal of a pull request. Nor do I feel there's an expectation that the code in a pull request is production ready. Often it isn't.

If you're using feature and release branches in a gitflow style branching strategy, then your pull requests become something more like release candidates. Not production ready, but something more approximating it. You know your code compiles (right) and you have sufficient test covfefe to make a decent claim that it meets the goals of the user story. And since you've already run several integration tests in your development environment, you have a great demo ready to go should you be asked to demonstrate your changes, which you will, during the review of your PR.

Ultimately, I feel that we should be providing assistance during reviews of the PR, but that assistance isn't around general coding. Instead, it's associated with preparing that proposed code for inclusion with a working base of production-quality code. The PR is an opportunity for developers to demonstrate they have a justification for, and a solid grasp of, each change they've included in the PR. And even then - even after we've weighed them down with unit tests, and demos, and loads of questions - there's still no expectation that the proposed changes are ready for production.

The code's close after all that. But then we let QA torture it.

Answer 5

I want to thank everyone for their contribution and helping me understand what people have to say about this topic.

This is my answer after the feedback given and my understanding now based on the answers and comments received. Thanks everyone.

Summary

• Not to expect or enforce perfect pull requests of the bat as this will only become frustrating for all involved.
• But continue to make it our goal for perfect Pull Requests.
• Expect some amount of collaboration/assistance in pull requests. After all, that's what you are essentially requesting by creating a Pull Request.
• If it gets a bit much due to design flaws or implementation flaws, spend time with that developer and do some Pair Programming to build them up and get their code closer to the goal. This is the role of all senior developers.
• Juniors will need more Pair Programming sessions than intermediate developers. So expect pull requests to reflect that requirement.
• Encourage developers to have design/implementation meetings prior to touching code to reduce any rework identified in Pull Requests.

Answer 6

Can you say more about your company culture in terms of the technical teams? If you're struggling with the idea of code being ready for production when a developer wants to merge into the mainline, then what are you really telling your developers? You're telling them that when their work is "done", it's okay if it doesn't work? It's okay if it breaks the system? If they are adding technical debt, maybe that's ok if they can justify it and are aware of what they are doing, and show that they can come back and do the refactoring later. But if they are oblivious to why they are doing something dangerous, how many times are you going to let it pass?

If you have a junior developer, their first few pull requests will obviously have issues. Eventually they should get the hang of it. If you're finding that they continue to have issues, then you may be assigning them work that they are not ready for yet?

Or maybe you need to hire a replacement junior and lay off the one that hasn't been able to figure it out?

You know what I've seen? People who are not competent developers continue to work at a company for years just because of nepotism. So of course their pull requests require multiple reviews. In Lean parlance, they are "waste"-- a drag on the team, and a drag on the bottom line.

So you have to decide for yourself: how many pull requests will it take for your juniors to show competency, and do you have the courage to let go the ones who don't?