Is it a problem that pull requests are approved without any comments

Question

I've recently joined a new company within this pretty small team. I'm running into this issue where my coworkers approve my pull request a few seconds after I ask them to review it. There never are any comments on it, no matter how complex the pull request is.

I'm getting a little frustrated with this because I know my code isn't perfect, duh, but I'm not being able to grow in my coding abilities if I get no feedback. I feel like I can just write any code and I get the approvals to merge it in.

We are currently without a direct manager so I can't talk to them about this. I also feel weird about going to the developers and asking them to review my pull requests better. Maybe I shouldn't?

Note that pull requests from other teams have tons of comments, good comments, not nit picks.

Not quite sure what to do here.

Answer 1

The short answer is: No

It's the job of the person executing the merge to ensure that the process has been followed. You did your job, now they have to do theirs.

You could follow up with them and ask why there are no comments or feedback on your pull requests if you feel there should be. This is a cultural question - how does your office work normally? Do people do actual code review? What do other people's PRs look like?

Answer 2

While the other answers are good, you have mentioned something new and relevant in a comment, which might be causing the specific behaviour from your teammates in your case. You said:

I'm also the most senior dev in the team

That could be important here. In that case, I can easily imagine that you are treated with extra deference / respect, and a mixture of the following could apply (I've seen these personally):

• the other team members don't want you to see them as challenging your code quality; and/or

• the other team members assume that what you write will be better than what they write and so they (wrongly) assume this means you won't make mistakes which they could find, even if they did review your code;

  and therefore:

• they just "rubber stamp" your pull requests - which is what you see happening.

This is especially true since you have:

recently joined a new company

So since you're new in the team, they may not know that the senior developer still wants in-depth reviews from less senior team members (not all senior developers do). Perhaps your team have previously had a senior developer who "blew up" when their code was reviewed, and so they have become conditioned not to do it?

I also feel weird about going to the developers and asking them to review my pull requests better.

I suggest taking one team member aside in a quiet private moment, and with no hint that they have done anything wrong, just ask them why they didn't review your code any deeper? And hope that they give an honest answer. Their answer might match one of my suggestions above, or might bring out something new e.g. perhaps you did something in the past (unknowingly) which made them think you didn't want in-depth reviews, and so they have been following that misconception ever since?

I don't think you will know the answer until someone in your team tells you why they behaved that way.

Answer 3

As the most senior developer on the team, I would say it is part of your responsibility to set expectations for the development workflow on your project.

I have worked on projects that are collaborations between senior and competent developers, where pull requests are pro forma unless a PR specifically requests that others take a close look at the code and double-check something. Detailed comments and reviews (especially on code style etc) were not expected or welcome for every little change. I've also worked on projects where every PR, even one-line bug fixes, are scrutinized.

In your shoes I would set expectations with a polite and humble nudge, e.g.: "I'm about to send you a pull request for [Feature X]. I'm a little inconfident about how I fixed [Issue Y]; could you please take a look and let me know if you spot any problem? Also, do you have any advice about how I might refactor [Section Z]? I think it might be hard to understand and maintain in the future but I don't see an elegant solution."

Answer 4

You're probably going to have to let it go until you get a direct manager.

Keep in mind, the person approving your pull request is trying to "do their job" - and apparently, in their mind, "their job" doesn't involve doing a lengthy review of your code. But that's the problem: you can't force them to change their opinion about what their job consists of.

Realistically, you're going to have to wait until someone in authority has the ability to direct what the priorities are - and is able to indicate to the other programmer that code reviews are a part of what they're supposed to be doing.

Answer 5

I agree with you this is a problem but I don't think the problem is that there are no comments. I think the problem is that the pull requests are instantly approved. That means no one is critically reviewing your code. And if they aren't reviewing your code, they probably aren't reviewing each others' code either. You aren't learning / improving, and bad code is landing in the main branch.

What you need here is allies. Figure out who on this team does take code review seriously. Put code review on the agenda of your next team meeting and talk it out. If the majority sides with you, more likely you win. If not, then even at most start-ups, there is some management. Get them to understand that this "buddy approval" issue is very real and a threat. Show them examples of instant approval or even of obviously bad code that was not caught.

With majority or management backing, go to whoever administers the repository and ensure that rules are put in place on the code repositories that require at least one trusted reviewer (who takes code review seriously) to approve any pull request before it is merged. All the other "buddy approvals" therefore carry less importance. However, I think it is still important that those who don't take code review seriously still be required to do it, in the hopes they improve.

I have battled this very issue on my team and having trusted reviewers was my solution. You have to protect the repository from habitual "buddy approvers". In my experience, if you can't make that happen, there is probably no way you can win the code quality battle. The best you will be able to do in that case is cover your own backside and wait for the inevitable disaster that forces the issue. Don't be the one holding the bag when it happens. Don't compromise your own code review discipline; maybe some of it will even rub off on them. Don't merge your own pull requests until you have more than the required number of approvals. And make sure you write better and more comprehensive unit tests than the next guy.

If you can do this without even the appearance of casting blame, then the next time a glaring defect that should have been caught in code review makes it into the main branch (ie code with bad anti-patterns), bring it up with the whole team as an opportunity to improve quality with a renewed focus on the code review process. Just make sure there is no blame, or you only hurt your own reputation.

Specifically concerning the need for comments, I think comments should generally only be made if you see a problem or have a question. If the PR is good, there is no reason to comment; just approve it. However, if a PR is declined without any comments, I think that would be a problem because the contributor does not know what should be changed to make the PR acceptable.

(And, this question probably belongs in Software Engineering SE.)

Answer 6

Do what I do - directly ask a senior developer if they can take a look at your proposed changeset and see if they are happy with it. Ask for feedback. I've yet to have a negative experience with this approach.

If they don't have time to take a look at your code, they'll let you know that.

Answer 7

I suppose I'm a little against the grain here, but yes this is a problem and yes you should attempt to be a part of the solution. Before going on, I'll say that this is a difficult problem to solve. Here's the principle to follow with code reviews:

If we are approving the majority of code reviews with no input, that means we believe we generally do things correctly the first time.

Generally speaking, that's a bad life philosophy. For software, we know that code which has been tested and deemed production worthy always has bugs. I had a professor in college that claimed it was about 1 bug per 100 lines of code in most software, but I can't find a source for that right now. Also, bugs are expensive: some claim that a bug costs $10k if found in production

It's worth it to spend some time reviewing code and at least asking questions about it.

As far as solving the problem, you obviously can't force people to change since you aren't management (and of course, management can't really force people to change). I recommend a few things:

1. Review code the way you feel code should be reviewed. In the conversations that follow, explain to people why you do it the way you do, and make it clear to them that you'd love for them to review your code the same way.
2. Discuss this problem with others who are influential among the engineers. At some companies, these people would be architects or other senior devs, but whatever the position is titled you are looking for technical leaders that other engineers on your team respect. Discuss what's happening with this person (these people) and try to get them to help.
3. When you do get a direct manager, make it a talking point with the manager and convince your manager that this is something that needs to change.

Recognizing these kinds of problems and attempting to solve them is a major differentiator among engineers.

Answer 8

You need to find out why they're not reviewing pull requests before you can decide what to do about it. Off the top of my head, it could be any combination of:

• They are deferring to you as the most senior.
• They have another process to review code.
• They have really horrible unsafe practices and just never review code.
• Your pull requests are too complicated to review in the tool.
• They are under pressure to release as fast as possible, skipping otherwise essential steps.
• They don't know how to do a real code review.
• They are too lazy to do a real code review.

I think the best case you could hope for would be if they have another process for reviewing complicated commits, but they haven't explained it to you and are quietly wondering why you aren't following it since you are more senior. In which case if you have a quick conversation they can explain how they do code review to you...and I would recommend giving their way at least an honest try before immediately declaring that your way is better.

I think the more likely, more unfortunate case is not good practices, not knowing better, and trying to skip steps to save time or effort. Then as the most senior it is your job to teach them and encourage good practices (maybe even enforce good practices, if you were hired as a lead). You will probably need to do code reviews as group for a while to set your expectations for how thorough they need to be. And you will need to adjust your velocity, since for a while you will be spending a lot of time on code reviews. (You make it up later by not having to spend time on bug fixes, but with no released product...yikes.)

Answer 9

If you're working in an agile environment, fixing this is your responsibility, as well as the rest of the team's. You guys are responsible for your own process, you own your work, you are a self-organizing team of capable software developers and should be improving in every way you can, both individually and as a team.

The core problem here is that they're either not following the process of reviewing every line of code, or the process is not well defined (for example it only states that PRs must be approved, not that the changes must be actually reviewed). Either way, you guys need to decide how you're going to work from now on, define a process you all agree on, possibly write it down (mostly for future hires), commit to following the process you yourselves just decided on, and do it. Bringing this up is the responsibility of every single member of the team, including you. Some (for example juniors) might not notice this opportunity to improve how you work, so bring it up yourself and teach them why this is good for the team as a whole (better code, less bugs), you as an individual (learn to write better code) and them as individuals (get used to reading other people's code, learn how well-written code looks like).

As others have mentioned, this might be related to you being the most senior developer. Some might think that questioning you is bad, and not realize that it's actually a great opportunity for them to understand why you do (most) things better than they do and how you think about problems to arrive at (mostly) better solutions. Teach them this. And also, strive to create a safe environment, where even if they sometimes don't clearly see opportunities like this one, they feel free to question anything and anyone without fear that the other person might feel attacked. You might want to ask around how other teams in your company do that, and how you guys can achieve that.

Answer 10

Your work should be properly reviewed before it is accepted. Since your co-worker apparently is unwilling to do it, you have to review your code yourself. It’s a good idea to do this anyway; the better the code you let others review, the better for your reputation and the less work for everyone.

That doesn’t mean you shouldn’t get him to review your code properly.

Answer 11

A few things from personal experience which can help with your problem:

• Keep PRs small and simple. Reviewing a small PR that solves a single, well-defined problem is a lot easier than going through many different changes in multiple files. The bigger the PR, the more likely it is that your peers will skim through it and miss important changes.
• If there are specific changes you want feedback on - comment on those and explain your concerns. This will draw people's attention to the change and prompt them to share their opinion.
• Tag more people on your PRs and require at least 2 or 3 approvals before merging.

Answer 12

My employer will assign two engineering leads to every project. One is the eponymous "Engineering Lead" who's in charge of tasking the project out and answering directly to production and QA, and is essentially a "Buck stops here" guy. The other is a "Principal Engineer" whose job role is essentially "Be the guy who knows everything."

Principal Engineers often set architectural guidelines for the project, dictate certain technologies be used (or not used), and enforce best practices in the code base. To a certain extent, that code base is their baby and they should not allow a blemish on the shell of any of its many many turtles. To that end, the Principal should themselves be reviewing some decent proportion of the PRs coming in, or at the very least be reviewing the reviews.

If you have an engineer in a similar role, I strongly recommend that you go to them with your concerns. They appear to be well-founded. If you do not, I strongly recommend that (given your seniority) you fill that role yourself, with the buy-off of your department head.

Answer 13

There are a lot of good answers here, but I didn't notice anyone that suggested that in your PR:

• Mention what you fixed
• Mention how you tested it
• Ask for them to look at/for specific things

---

Another thing that you could do, and I do this with my own PRs, is after submitting your PR review it yourself.

That means taking off your, "I wrote this," hat and putting on your, "Someone else wrote this," hat.

Pretend that it was your most junior dev on the team who wrote the code, and review the code exactly like you'd expect it to be reviewed. Comment on your PR the way you'd expect to have people comment on it.

There have been a number of times when reviewing my own code that I've discovered things that I forgot to fix/debugging code I forgot to remove.

It also sounds like you do have a cultural shift you need to make. As the senior dev you have some weight to throw around, but you need to decide if you want to expend the political capital to require things by fiat, or if you want to spend the time to build the team skills that you're looking for. The latter is probably more effective, though it may take more time.

Answer 14

As this is Workplace.SE not Software Engineering SE, I will answer this question from a general "new engineering hire" point of view.

You're in a new job on a small team. As such you need guidance on the way things are done in this company and in the project you're working on. But as a small team who has probably had to deal with being short handed until you were hired, it's likely they have a backlog of work that needs doing. Have you asked this?

We have a new hire in our company (engineering, not software.) At first he was only assigned one project - one of my projects - and was clearly frustrated that he was not getting the supervision he would like. But I had urgent things to do (since we have been short handed for several months) and could only help him with things that need to be done perfectly right NOW (i.e. stuff that was going outside the company to the client and suppliers). He proactively took it upon himself to prepare bill of materials for internal company use for the project (not on the company form as he wasn't shown where it was.) His format turned out to be a vast improvement on the company form, so I told him to stick with it but make some adjustments to the content. He now has several projects to work on so the issue is resolved - in fact just a couple of weeks later, he's too busy to accept my guidance when I offer it and we have to set a mutually convenient time.

Point is, you need to find out if what's happening is a temporary situation or if it's what they always do there. They clearly feel that writing "their code" is their responsibility and more important than reviewing "your code" which is clearly wrong. Document what is happening so that when someone complains that your code doesn't work or meet company standards, you can counter with "I was new in the company and nobody gave me any guidance." That's the best you can do.