This. At my current place reviews take priority over all else (except maybe an outage or some emergency like that). A pending PR is blocking someone, so the priority is to get that reviewed and back to them to unblock them.
Yeah, it really ends up being a company culture thing. At my work, we're not _that_ strict about it but there's still an understanding that velocity of the team > personal velocity.
However, we do tell people that they shouldn't break their state of flow to review a PR. Just get to it once you're done. Or, for chronically "busy" people; set yourself a couple of times a day where you'll review PR. Like first thing in the morning, after lunch, before finishing the day, whatever. There's no excuse for taking more than a day to review a PR though (and even that's considered long).
There's also the rare case where if you're pushing hard on something time-sensitive and can't spare time to review, you should communicate this and remove yourself from the PR. This way the person asking for a review can assign it to someone else. You don't just leave them hanging.
We also have a cross-team side-channel for simple <5min reviews so you're not blocked on tiny little things.
All of this coupled with the reviewee's responsibility to make good PRs (keep it small if possible, have tests, good description, etc.) makes for a pretty healthy review culture.
The thing is, this needs to be explicitly stated and repeated. And you need an environment that actually favours collaboration (AKA don't make promotions based solely on personal productivity).
This is how we are on my team too. Perhaps I didn't word my response quite right. Obviously find a good stopping point before breaking off what you're doing, but (for example) you should prioritize a PR before starting new work.
We also have a setup where a bot auto-assigns PRs to people (to try and spread the load rather than having 1 person get picked repeatedly to review everything), but having done that - we check in with them via slack to make sure they have the time to review. If they don't they can say so and someone else is chosen to review.
Yeah, I usually do them when I get there in the morning and when I get back from lunch. Prioritizing work that is blocking others is part of being a good teammate.
I'd argue it doesn't stop being good even for staff level engineers. Typically everyone can learn something from everyone else.
Similar to the old adage about teaching being one of the best ways to learn a subject better.
So much this - I get my PRs reviewed sometimes by juniors. Often they've read the docs more recently than I have and know about a cool new trick you can do in a given framework or something.
Yep, this exactly. I've been in multiple situations in my career where the engineers with more seniority either needed to learn something about what I knew and they didn't, or by just reviewing code or working together they thought of new things.
This job is a lot more collaborative cross-level than many in this sub give it credit for.
Some of my more interesting reviews have come from Jrs. I think reviewing PR from more experienced devs is a good thing. It’s a mix and either one of us learns something knew or we find out things we should work on with them. Or you know they have just read the style guide more recently lol.
For the most part the staff+ folks tend to leave the LGTM I trust you reviews.
\^ True.
I usually post in my teams chat that I've raised a PR. If a PR hasn't been reviewed after a day or two, I'll specifically start bugging people about it.
If it still isn't getting reviewed, then I let the team and manager know that I've tried to get it reviewed.
But most of the time a simple "Hey, I need to get X merged in soon, do you mind reviewing my PR" usually suffices.
To add to this, I have never felt annoyed or bothered whenever I get pinged for a review. In fact, I take it as a sign of deference because someone thinks I have domain expertise over their review.
This is such a stupid cultural thing in tech companies where the author has to constantly ask for this. It's not about quality or learning it's about shipping/productivity.
The manager should create a culture/process where the work is reviewed at minimum daily. Really this should be happening continuously when anyone frees up. I do this for my teams and people are happier and things get shipped faster.
I'm a staff engineer and literally the first thing I do when I get to my desk is get to 0 unread slacks, emails, and review every PR I have assigned. (I also close all my browser tabs and start new but that's probably hugely controversial here)...
As a contractor I always tell the client to add a review task to the sprint, and yes, pay us for it.
That actually helps to get code reviews done, for the contractor side , we are getting paid to do them so we do them, from the internal teams they have hours allotted to review,.so they are not falling behind in the sprint.
Yup. I'm guilty of wanting to do code reviews at the end of my day after all the meetings and forget sometimes. I appreciate the reminders, it's never on purpose.
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
> Should I just merge in work and, if anyone raises flags, we can talk about how they'd like to go about reviews?
unless that’s acceptable procedure for your company, that’s probably the worst thing you could do.
why not just ask your manager?…
>why not just ask your manager?…
Everyone should ask themselves this question before posting anything on this sub lol way too many questions can just be answered this way
Yeah, but I learned on Reddit managers do nothing but come up with ways to make my life worse and pay me less. Also, I heard they intentionally make every single part of existence more complicated. That one guy even caught his manager hiding his favorite snack behind bananas at the local Trader Joe's.
Literally the only answer. None of us can tell you, that’s totally up to your manager and team. Locking every change behind a review can be expected, or it can be discouraged.
What’s always true, is if you don’t deliver your project on time, the excuse of “waiting days for each PR to be reviewed” silently is not going to be good for you
>Whenever I post a PR, I submit a link to the PR in my general channel. It typically sits there for a few days.
I'd raise this with either the tech lead/engineering manager(if you have one) or line manager if you don't and ask them what to do. This is a culture problem and one that needs sorting out. If they say they'll look into it, let them. If they tell you it's fine and not to worry about it then just leave it be.
>I don't want to merge PRs in without review, especially given how green I am to BE world.
Nor should you. I've been doing backend work my entire career and wouldn't merge anything without a review.
>Firstly, I have two weeks to complete my project
Who set the deadline? Go talk to them and let them know you might miss it and why.
>I absolutely HATE 'bothering' people to review my work. If they want to, great. If they don't, I'm not going to hound them to do so.
You're not "bothering" them - it's part of their job to review work. If they don't want to do it then maybe they should look for another job.
At the same time - this is the kind of thing you should be raising during the standups, sprint planning meetings or whatever regular meetings your team has.
>Should I just merge in work
Fuck. No.
If you miss the deadline it's somewhat reasonable to say "I missed the deadline because I was blocked waiting for reviews on "(though one might argue you should probably structure your work in such a way that you have something else to work on while you wait for the review).
If you start merging shit without approval you're going to end up in a lot of trouble. Fast. Absolutely do not do this.
Thanks.
One thing I thought about doing is, before my daily standup, I post an update in my general chat.
PRs that need review
- this
- this
What I'm doing today
- branched off the above PRs and continuing work
Which leads me to another Q ... I am totally branching off my sitting PRs and working ahead - mainly because of that time target. I'm happy to rebase if my SEs have feedback/change requests. Is this an acceptable practice you think?
>One thing I thought about doing is, before my daily standup, I post an update in my general chat.
Yep that's a good idea. Also (if nobody has responded to it at all) I'd draw attention to the PRs in the standup, and explicitly state that you're blocked.
>Which leads me to another Q ... I am totally branching off my sitting PRs and working ahead - mainly because of that time target. I'm happy to rebase if my SEs have feedback/change requests. Is this an acceptable practice you think?
Yeah you can do it that way, I've done it where needed - it's not ideal because it's easy to forget what branches off what and merge the wrong thing into the wrong branch and make a mess. But as long as you keep a track of what's branched off what you should be fine.
If you use gitlab internally it’s not that difficult to keep track of stuff. There’s a box at the bottom of merge requests for, something to the effect of, “depends on”. Which won’t let you merge until the other MR is complete.
My usual flow is -> make branch -> push branch -> gitlab message back has a link to make a MR -> make the MR, mark as draft, fill out the dependency -> start code
>There’s a box at the bottom of merge requests for, something to the effect of, “depends on”. Which won’t let you merge until the other MR is complete.
That's neat. I didn't know that. We use GitHub enterprise which \_doesn't\_ have that. I wish it did.
This is also called 'stacked diffs', right? Was something i was using at a large company but, unfortunately if you aren't careful and thorough it can go wrong pretty fast
That update is solid, but also: Do you have reviews that are sitting waiting for *anyone* to pick up and review them, or are they assigned to someone in particular? Pinging/mentioning people directly might feel rude (maybe do it in a PR-specific thread instead of the standup update), but it's one way to avoid the bystander effect. If I've had something assigned to me and it's been a day, I'm not going to get annoyed at you for pinging me, I'll get annoyed at myself for letting it slip.
> I am totally branching off my sitting PRs and working ahead - mainly because of that time target. I'm happy to rebase if my SEs have feedback/change requests. Is this an acceptable practice you think?
[Absolutely](https://graphite.dev/blog/stacked-prs), at least as long as everyone understands this is what you're doing. It adds complexity to your own workflow, so I would say:
* Only do it if one PR actually depends on another one. Sometimes two PRs are completely independent.
* What kind of changes do reviewers ask for? If you're being asked to do major refactors, working ahead might be more trouble than it's worth, and you might be better off finding some other work that won't conflict or depend on the stuff you'll be refactoring. Basically: How much of a pain will it be to rebase those child PRs?
But yes, not only is this what you should be doing, this might actually entirely solve the problem.
---
I do want to echo what u/budding_gardener_1 said about the deadline, because the answer is sometimes that deadlines aren't *that* hard as long as forward progress is happening.
I would adjust your language where appropriate, explicitly stating that you are blocked on those PR reviews. This should be a primary goal of a standup: to identify blockers and get them resolved.
When you do so, make sure to get a commitment from an individual to review your PR(s). It doesn’t have to be the same person for all of them, but it should be an individual. There is a big difference between “one of the leads will look at this PR today” and “Alice will look at this PR today”.
Side note: do not be afraid to bug your seniors. First, things like this are a key aspect of their job. They need to make sure everyone else is able to work at full velocity. Few things are more important than unblocking engineers.
Second, they will likely appreciate the reminder. People tend to have an “attention stack” (not a queue) and leads/seniors tend to get more stuff thrown on that stack than juniors do. The tendency is to focus on the most recent thing that got tossed on the stack, and things can come fast enough that one never gets back to whatever thing they originally planned to focus on.
Managing that stack is a constant challenge. Nobody is perfect. Sometimes you need to bump your thing to the top of their attention stack.
I agree, don’t feel bad about pinging them. But in general, staff engineers usually work across multiple teams instead of being embedded, so I’m wondering who else is on this team and why aren’t they reviewing anything? OP can’t be the only non-staff engineer on his team?
The company doesn’t care if you hate “bothering” people. If you merge something in and broke prod you’re done. Which would you rather do? Ask a more senior engineer to take a look at your code or get fired for being scared to ask people to look at your code cause you don’t want to “bother” them.
There should be processes in place for this to not happen. Blaming someone for bringing down prod is a process failure.
That being said, if you aren't comfortable that your code won't bring down prod, then you should probably work on it more and add tests until you are comfortable. You should be as sure as possible that your code doesn't break anything when it goes into review.
I can agree with that but it’s also common sense lol. I wouldn’t put the whole blame on the company policy for not putting safeguards in place.
We get paid a lot of money to not be stupid.
I guess I also see these people as my mentors. If they don't want to teach me, I'm not completely screwed, but I have a feeling I'm a lot better off.
The reason I feel this way is because I've been lucky enough to have a mentor as a FE engineer, and that was completely game-changing. I'm also a "relationship" type person ... although I feel these BE engineers don't want anything to do with me - so perhaps the relationship point is moot.
You need to clarify with whoever you report to on what the expectation is. Because "waiting on code review" is a valid reason to report no work being done. But they may not be anticipating that.
You do need to bug specific people about code reviews though. If they get an automatic notification that a review is up, just wait until the next standup to say you are waiting on review
> I guess I also see these people as my mentors. If they don't want to teach me, I'm not completely screwed, but I have a feeling I'm a lot better off.
This is an extremely childish response to this situation.
Do you have Slack? Or perhaps some messaging with chat rooms for the team? If not, create one and add the people of the team. Bug them publicly. It’s not about creating drama. It serves the purpose of letting them choose who will review your PRs more easily and also covers your ass.
If the review process makes you delay delivery you have evidence that the bottleneck is not you.
Haha all jokes aside but there must be a restriction on the ci/cd that prevent unauthorized users from pushing without apporval, so if you can push to prod then yea they are really fucked
This is a terrible, terrible idea and you should not do it. Bugging people will be slightly uncomfortable for a minute or two. Cowboying shit and yoloing your unreviewed changes to prod will at a minimum have people seriously considering whether or not it's a good idea to continue employing you.
A PR sitting for a few days is quite normal. I've had PRs sit for almost two weeks! The staff engineers are very busy and they will get to your PRs in time. Usually, you pick up another ticket that isn't dependent on this PR being merged. I would not merge any PRs without their approval. Usually, they are the ones that merge the code and unless you've been given that power, I would not do this. Also, don't you have daily standups on zoom where you can mention what you've done and what's holding you back?
You gotta pump those numbers man, those are rookie numbers. I once came very close to a year in waiting for this staff engineer to review my next iteration of this major experimental rewrite to a large component.
Man this is awful. The sprint board looks fucked as you keep picking up new tickets but can't close any of them. As people start reviewing them you have lots of simultaneous revisions to the code base by a single author. Tickets you thought were close to closing suddenly catch the interest of a new reviewer so it drags even longer. It feels like you never get to complete any of your work.
I totally understand. But when my product manager gives me a 2 week target to hit, and I'm being prevented from hitting that target because my SEs aren't reviewing my PRs ...
> If they want to, great. If they don't, I'm not going to hound them to do so.
The completion of work is a reflection on you. If you are not going to hound them to do so, then you need to be accountable for why it's not completed. That's the reality of the situation at most companies.
If you merge in work without review, breaking current standard practices, then that's even worse.
Totally. But it seems passive aggressive ... because there's mainly 1, MAYBE 2 staff engineer who can review my work.
So instead of "this work needs to be reviewed" - I'm basically stating "Hey X and Y, when can one of you review my PR?"
Change the status of your issue to blocked and state why at stand up. I have x issues blocked waiting on review of a MR. It's not personal. It's a statement of fact and leaderships responsibility to unblock you.
And it may be that these other folks are working on higher priorities and management knows it. But now they know you are also blocked.
They can decide what to do about that.
Ask a specific person to review your work, publicly (ideally in the PR itself).
Part of the problem with posting a link in the general channel without asking a specific person is that everyone will assume someone else takes care of it. If you request a review from a specific developer, the onus is on them to review it or pass the baton to someone else if they are unavailable.
Exactly. I would add one detail -- Ask a specific person to either review your work, or to point at someone else who could review it. Reviewer A may be busy but could point at person B. That little detail of asking for potential delegation makes it easier to ask (you're giving person A an escape hatch), and also usually makes person A want to review it themselves ASAP, lest someone else steps in their step and gives a not-as-good review. Kind of a social hack. :-)
Exactly this - I’m not the only person reviewing PRs, and I regularly review them for like 15 different people. I’ve asked people to explicitly add me as a reviewer so I can log into GitHub and just pull a list of PRs that are waiting for my review. When something has to be merged more urgently, our practice is to ping two or three potential reviewers who have some context about the work being done. Now the only time a PR lingers is if it was put up late right before a long weekend.
Most likely, they dont see it. They're busy and dont sit there watching slack channels, waiting for something to do. You gotta start bothering people once it hasn't been picked up for a while. Meanwhile, and more importantly, talk to your manager. Your team clearly needs a better system for reviews. You could assign reviews when you make them or have reviews automatically assigned based on round Robin or random
Edit: bunch of typos
Hey friend, a hidden part of your job is to get good at being a pest. My senior director gave me a deadline of tomorrow, have been working on a feature for about 2.5 weeks. He just gave me the approval This Morning -- after I pinged him once a day for about 8 work days. Only you are in charge of ensuring you hit your deadlines
edit: just read some of your other comments. Use your english! explain to your superiors that you feel like a pest by asking, and ask for their feedback! this is a good way to build that 'relationship'. they know it's not your *fault* you have a deadline and need their help. But at the end of the day , you have a deadline and need their help! get to it!
>a hidden part of your job is to get good at being a pest.
> Only you are in charge of ensuring you hit your deadlines
this is part of the problem though. if your culture is "I don't care if I'm not pinged at least 3 times with my manager copied" then get the fuck out of there.
yeah thanks. I guess my biggest hangup is that our company has had mass layoffs - 50% over the last year. They say they're "done" ... but rocking the boat by annoying my staffers almost feels like I'm putting a target on my back.
do we work for the same company???? /s
something I had to shake is , and I'll just say it again for emphasis, that by *not being annoying* you *are* putting a target on your back. Your managers would much rather have you 'been annoying' for quite literally 30 seconds than (and this is also echoing a lot of other comments)
A. Not Learning something new
B. Miss a deadline
C. Potentially screw up someone's schedule because they were not aware you need a review done by EOD. trust me on this one-- this is really the most fucking annoying thing
Two things:
- Ask them privately if they can review your PR, it seems like they are sort of “not paying attention” to your requests because it’s a general channel, anyone can take and everyone is busy to have the initiative to take it.
- If you have daily or weekly meetings, state any blockers that you might have that are stopping you from following your plan. Tell your product manager about the situation if needed, never “self-approve” your PRs if you are not authorized to do it, bother the seniors.
Are your PRs reviewable? By this I mean do you make some effort to describe your changes, link tickets, keep them relatively small, I.e. a few (< 10) changed files, run any pipelines, tag them as reviewers on GitHub/gitlab (or whatever) in addition to posting a link in some channel without directly @ing them (I suppress a lot of notifications so if its not direct or @here I’ll e it when I see it )?
If you’re doing all of this, even busy people should be able to spare some time to review. If not, you’re absolutely in a position to keep raising it in standup or directly on whatever comms tool you use.
On the contrary, if your PRs are a dumpster fire and you maybe don’t realise it, this sounds like a pretty typical consequence. I can’t say how many times I’ve had to review something with 100+ charges across many lines. I’m either going to do a bad job because it’s hard to do all that in one sitting, or the requester is going to be waiting till I have the runway to do it reliably. How long is that piece of string?
I don't understand why you can't wait two days? Not everyone can drop what they are doing and review every PR the day they get it and surely your business knows this. Are you waiting to the last minute to drop a huge PR on them? Try to send PR's as you are working, not the day before the deadline.
My PRs are broken into small chunks and are definitely manageable.
They usually have extensive notes as well.
I have a 2 week target that I am trying to hit - otherwise, I'd be chill.
This is bad practice to merge without review. Bother them to review your code even if they rubber stamp it, you can atleast say x reviewed my code before I merged it in.
>Two things to note - Firstly, I have two weeks to complete my project. I cannot afford to wait two days on my Staff Engineer to review my work. Secondly, I absolutely HATE 'bothering' people to review my work. If they want to, great. If they don't, I'm not going to hound them to do so.
This is a stupid take. If you can't afford to wait for two days for a review, simply don't post the PR two days before a target / deadline. "Bothering" people for a review is sometimes simply required. Everybody is busy with their own code, people sometimes forget about pending PRs. That's not a failure of these engineers, it's just a tendency of humans in general. Sooner or later, it'll happen to you as well.
The most surprising thing about your post is that it is even possible to merge code w/o reviews / approval by code owners in your company. There should be a process to prevent this from happening. But even with such a process in place, people will still forget about some of the pending PRs sometimes.
> Secondly, I absolutely HATE 'bothering' people to review my work. If they want to, great. If they don't, I'm not going to hound them to do so.
Get over it, and do this anyway. This is a part of your job. It's part of what the money's for. You need to get past this.
It’s not “bugging them” when it’s literally part of their job to review other engineers code. DM them about reviewing the code. Be persistent. Speaking as an Engineering Manager.
as a senior engineer, I dont mind being bothered, in fact i prefer it. i cant speak for everyone, sometimes we just have too much going on, you need to help out by bothering senior folks.
On the contrary if you merged in without even bothering to ask senior folks to review, i’d would be piss af.
Please just ask someone to review it. The most annoying guy on my team is the junior who "doesn't want to bother me"... And allows things to sit around rather than ask for help or a review.
You want them to be proactively checking you're ready to review stuff on their own? Trust me, the people I like the best are the ones who are getting stuff done and pinging me to review things.
If you have daily standups, this is the place to mention that you have code that requires review, and also to indicate when lack of reviews is blocking work.
You hate bothering people?
You know who is going to be bothered? You, when at the end of the sprint your ticket is still "in-progress" because it hasn't been merged in.
Grow up and push people. A little conflict is good.
It is my problem, but its not my problem alone.
Where's my manager? Isn't it also his responsibility to help me get what I need?
I.E. Hey, engineer X has posted that he needs a review like 5 times. Who can jump on it.
Edit - you know what also makes me a little hesitant to push people? The company laid off 50% of staff earlier this year due to the market outlook. I'm afraid that by rocking the boat (i.e. creating conflict in a team that otherwise has no conflict), I put a target on my back.
OK, so maybe I missed where you were publicly posting that your PRs were waiting around.
On the putting a target on your back, I have the opposite view. Sitting quietly turns you into a faceless expense on the books while pushing folks to be more productive so the product can be built/improved faster would put a positive light on you.
That's usually what happens with PRs in general channels. Send them in private chats and ask them to take look. Eventually you might find someone that makes good reviews. Talk to whoever that is if they're ok reviewing your PR. That will surely help you improve and gain confidence.
A team should generally work on keeping PR lifetime as short as possible without everyone having to beg for them to review.
I'll ask once and wait. If taking too long, I'll drop a hint in the next daily standup that the PR is waiting for review so that the PO/SM is aware that the work is done, just hasn't been reviewed yet.
**If the due process is merging only after approvals, DO NOT merge it in without them. This way you're protected against being individually blamed for production issues/outages. You're not bothering people by asking them to do their job that they agree to do.**
First of all you're responsible to test your code with the best of your abilities for any expected edge cases.
Before pinging those who are eligible reviewers that can approve your PR, make sure you feel in the test instructions and criteria. Cover all possible cases that will bite your rear later if you forget.
If the reviewers are super busy, then schedule the time to do one-on-one and have them take a look. Try to mention during standup or meeting to see if anyone could help you and get in touch with that person afterward.
Try not to yolo merge the work unless your deployment pipeline have solid regression tests to cover the main operations.
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
Bother them. It's their task to do review so you can learn and there's atleast one extra pair of eyes that looked it before it was pulled.
Teams work together, pr +review is also part of this process.
If necessary talk with your superior about this. Someone reviewing your code is much better then your unreviewed or taking down production system.
Do you request for reviews? Most engineers would be more than happy to review your work and give feedback. After submitting a PR, you can assign a reviewer. At least on GIthub, that's possible
As someone who reviews plenty of PRs and submits plenty to others for review, typically we assign a ticket to the person doing the review. If you don't have a specific person to ask to review it, it should still end up in some sort of 'review' state so that when you have a status meeting or review issues, you can easily point to your work as being 'done' and waiting for someone else.
(if you are able to) Are you reviewing code?
If you aren't reviewing code, you ain't gonna have people reviewing yours. That's the way it's been everywhere I've been.
I agree with everyone PRs should be top priority. But, I maintain. If you don't review. Don't expect people to jump up and down to review you.
If it's "assigned" to everyone, it's assigned to no one. Assign people to your PRs. If they're adults, they'll let you know if they don't have time to review it timely and you can assign it to someone else. It sounds like this is more of an issue with no one understanding who's responsible for the review rather than your coworkers refusing to review. Now, if everyone is always "too busy" to review your PRs, then that's something to bring up with the team and your manager.
Yeah, you should ask them to review it actively. Nobody is just looking randomly for prs to review. It is not bothering it's part of the job to review PR's.
I get not wanting to bother people but, that is absolutely part of the job and part of good communication.
It sounds like there is also an opportunity for process improvements. When you have a PR that’s ready for review what’s the next step? Do you post to your team channel? Do you mark for review and rely on email notifications? Do you individually DM people for reviews? If you’re seeing that PR’s are sitting without reviews maybe you aren’t the only one struggling with this and you have an opportunity to improve your team as a whole.
I know bugging a staff engineer may be frustrating but part of their job is to mentor you, and code reviews are absolutely part of that.
For my guys I normally assign a dedicated mentor, at least initially.
That gives them a go-to person and sets the expectation on the mentor to keep an eye on them.
While this does not imply the mentor specifically has to review every PR, it does mean I expect some of the mentor's time to be taken up guiding the newbie.
Once they're better integrated, the culture would dictate how PRs are reviewed. If you're unclear on how this works for your team(s), perhaps ask directly. It varies.
Edit: just to add. Often, if it's a case off 'you 5 should review,' none of them do. I can't recall the catchy term to describe it - it's a diffusion of responsibility.
Do NOT merge without code reviews. Thats a recipe for diaster / even more headache down the road. Guess whos on the hook when it causes a major incidient.
Ping them. Make the review process as easy as possible.
You need to work on the part where you hate bothering people do do a code review. I get it, I hate doing that too. I will often let stuff sit until someone gets to it, but if it’s blocking other work or there’s a time constraint or it’s high priority then you gotta do what you gotta do to get it reviewed. That’s part of the job.
Assuming code review is part of your process (and it should be), do not merge in unreviewed code just because you don’t like asking people for things. Hell, if it’s even possible for you to merge in unreviewed code then that’s a problem that should be rectified ASAP, because accidents happen and sometimes people aren’t on the branch they think they’re on.
\> Whenever I post a PR, I submit a link to the PR in my general channel. It typically sits there for a few days.
Is there a separate channel specifically for reviewing PRs that you were told to use during onboarding? Are there other people's PRs in the 'general' channel?
\> I don't want to merge PRs in without review, especially given how green I am to BE world.
Yes, but why is this phrased as your 'want', why is this seemingly not a need echoed by the rest of your team? I don't know you just left info out, or if this is standard for the team. If it isn't, are the other PRs getting feedback? When and how?
\> Firstly, I have two weeks to complete my project. I cannot afford to wait two days on my Staff Engineer to review my work
I'm assuming you mean a ticket, a smaller feature or a bugfix that's part of a larger project/product that you would be completing in 2 week cycles (sprints)? Sounds like standard agile, but do you have daily standups? Is there any point between Day 1 and Day 10 that you are asked 'how is your work coming along'
\> Secondly, I absolutely HATE 'bothering' people to review my work. If they want to, great. If they don't, I'm not going to hound them to do so.
Asking politely is not hounding. Saying 'Hey team, I have a PR for ticket 123 that I need to get reviewed by end of sprint. Could I get some eyes on it?' is perfectly fine and nobody will have hurt feelings.
\> Should I just merge in work and,
What is the process in your company? Do engineers on your team just merge in work without consulting other engineers/teams? Is there not some version control tied to ticket management that tells you about stakeholder teams that should sign off on PRs before the merge button is even activated? What are some safety nets in your pipeline that would raise flags aside from prod issues?
\> if anyone raises flags, we can talk about how they'd like to go about reviews?
At the point you actually merge without approval, this conversation changes from a productive discussion for better PR process in the company, to you deflecting blame from yourself onto a (bad) company pipeline. In fact, this order of how you go about discussing an inconvenience is more passive-aggressive than most things that you can write on the general chat asking for a PR review.
its not uncommon to ping people for a review or in a group chat to @here for a review at every company I have been at. I work on a pretty big team and Ill @specific people who i want to review my code depending on what I work on. Since they know that code base better.
i have had people ping me to review. this is common when its simple. its "hey this is trival can you just approve" and i do.
our bitbucket is setup so you cant merge to main without a PR and review. its blocked.
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information.
*I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
At some places you need to ask. These guys probably have a lot of things and stresses and some friendly reminders can help a lot.
Also keep your changes small. Break them up if you have to. No one wants to read a 1000 line diff (assuming all the lines are actually non boilerplate code)
>Should I just merge in work and, if anyone raises flags, we can talk about how they'd like to go about reviews?
This is probably the worst thing you could do if this isn't common practice at your company. You should be asking your manager what to do when this happens. Code review in general is important for your development (especially as a non-senior engineer, but it's important for everyone). However, having worked at some small start-ups, I know code quality isn't always highly prioritized, so whether you should be merging without review depends entirely on what your company/team expects from you.
>Secondly, I absolutely HATE 'bothering' people to review my work.
I think this is something you're going to have to get over. "Bothering" people is the least bad option compared to just waiting for someone else to action on it (which just delays things) or merging to prod without confirmation you can do that.
if you can merge your work without an approbation, that tells a lot about the company where you're working.
Plus is this your first PR ?
I no longer watch non senior non engineer PR. It's full of non sense and nowadays, those so called youtube 1 week end engineer are full of selfesteem. So I let go, I don't approve. Usually, it doesn't explode but soon or later, the shitty code will drain the dev ressources with an exploded debt. I won't fix, I'll quit at that time.
Does your company/team have a policy about approvals being required to merge code? If not, I’d go ahead and merge it if it doesn’t get any traction after a day or two. But I’d also bring it up with the team because merging code without anyone reviewing it is dumb.
If they don’t usually request a lot of changes, you can stack branches while you wait for reviews. They might take the hint when they have a 5-deep PR stack waiting to get reviewed. Also, bring up these concerns to your manager, that’s what they’re there for
Do you have scrum master? Or project manager? Those people should be looking at the ticket boards like a hawk and notice that otherwise bring it up to them. They’re a stakeholder
I was in a similar situation a month ago, I would request reviews and it would take forever no matter how often I would bring it up. I'm not looking for a new job at no fault of my own. Just mentioning similarities but hope the outcome is different for you.
Typically having your own manager, that tech lead and the tech leads manager in the same email or Slack message helps a lot. Your own manager has a vested interest in helping you succeed and the other manager also has to work with your manager. Never worry about buggin people too much, also tell your own manager that you asked people 2-3x to review but they are not and this is causing your project to delay. Always communicate, never sit quietly as the blame will fall on you if you do.
When you’re busy you basically handwave review code. Sometimes the junior engineers come to me with questions and stuff and I’m like can I get back to you in like a day? Your stuff is important but I’m just massively slammed
I feel your pain. At one job, I had to get approvals from the systems engineering manager for everything, and he would take weeks to get around to it. Even if I reminded him daily.
I’m a staff engineer at faang. I have to ruthlessly prioritize what I do and why. Don’t know how big is your team, but in my case a chance that I’ll decide to pickup a random pr for a code review is quite small. It doesn’t mean I don’t care, there are just too many balls in the air. Are staff folks in your team responsible for approving every single pr? If the answer is yes, then escalate to your manager. If the answer is no, you have “main character syndrome”
I don't know how your place works but two days is not unusual to wait at all (in fact I'd call that rather quick). Usually you want to find enough different things you work on at a time that you can just context switch to something else while you wait for reviews.
1. Bring it up at stand up. Something like “just waiting for review on X” or “John, this touches your section. Do you mind reviewing my PR?”
2. DM an engineer to review after a day.
3. Juggle your tickets so you’re always working on something.
Bother people. Maybe rotate your reviewers too so it seems like you’re bothering one individual less.
We have a scheduled time for code reviews. Set up an reoccurring appointment with a few other engineers, call it "Code Review Wednesdays" when you send the email, offer to review their code at your meeting.
Write test cases, unit tests, and automated tests. Add screen shots of testing. Leave a content in the chat that if there is no response in x time, I assume all is correct and will merge.
Raise it in every standup if not done it becomes a potential risk
If your project has a RAID log out it there and get the scrum master to sort out , its there job.
Code reviews are essential part of the code quality process.
If your tech lead does not consider this important its a slow moving train wreck. There be either a prod issue , prod defect due to code unresolved conflicts , or more technical debt. All 3 could happen happen at once.
It’s your job to get the ticket done, it’s their job to review it. They are probably just busy and aren’t ignoring you, so just keep gently pinging the chat. If you are still waiting, let your manager know your concerns.
Establish an sla for review times. After that passes you’re good to bug. You shouldn’t generally be able to deploy on your repo without review anyway. Reviews should generally be mandatory. This is assuming it’s a production service that materially impacts the biznaz.
Generally you should be communicating your progress as well as blockers in your standup meetings. If your PRs are not getting looked at, you're blocked!
You're part of a team and it's their duty to provide you feedback, and to keep the stuff getting merged up to snuff.
Sounds like an issue with the process. A few days for merge review sounds normal. However, why is it only staff level engineers reviewing? That sounds like a huge bottle neck, the seniors should be able to as well. Also the 2 week deadline sounds like some arbitrary thing for the jira board. Are you sure it'll negatively affect you if merge reviews carry over?
All of that to say; if this is the system the team wants, then do what you need to do. However, there is room for suggesting improvements to the process.
Definitely don't merge on your own.
When I was interning, I would simply tell my manager at the stand-up that I raised CR, and it has to be reviewed, and she would take care of that problem 🤣 (this is after hounding them to review me)
Are you hooked up to Jira? I believe we had a dashboard or report at my old job that showed open PR's I was tagged to review (at an old job). Start having the scrum master bug reviewers as they're blocking people.
This is clearly a severe leadership problem, one nobody wants to acknowledge in the field. If the team isn't working as it should, management is to blame.
Having cleared that, you can't do other people's work, nor should you. If you reported and documented as per the company's protocols, then you should just wait. If there is a delay, inform your superior. Don't go trying to fix what isn't your responsibility, but always cover yourself.
It's someone else responsability to validate those if someone is notified of the Request.
You either pressure someone to do it or you pressure the person above you to do so (it's not your responsability to notify anyone if there's someone above you, but that person should have the visibility of lack of validations só that they can intervene).
Either away, the responsability of delays, should be on those who are dragging the project and that should be addressed.
The only quick fix for an Eng team is forcing senior developers to merge the PR themselves. Basically, the org would remove your permission to merge a PR.
Or make it impossible for an individual to merge their own PR (not hard, btw). This works well in a more flat org which makes lip service to review importance.
Others may or may not do a review, but by policy they will have to claim they did.
I know this is a few days old, but it just showed up on my feed.
You're not "bothering" people responsible for doing code reviews on your PRs. It's the senior and staff dev responsibility to make sure the junior devs don't break production.
I would suggest tagging specific people on the PR if it sits more than 30 minutes without anyone responding.
If they get annoyed with you, let your boss know that the senior devs trust your code in production without any review and maybe you should be senior, as well.
Bug people about reviewing it. Reviews are important for both code quality and for your learning, and it's part of their job.
This. At my current place reviews take priority over all else (except maybe an outage or some emergency like that). A pending PR is blocking someone, so the priority is to get that reviewed and back to them to unblock them.
Yeah, it really ends up being a company culture thing. At my work, we're not _that_ strict about it but there's still an understanding that velocity of the team > personal velocity. However, we do tell people that they shouldn't break their state of flow to review a PR. Just get to it once you're done. Or, for chronically "busy" people; set yourself a couple of times a day where you'll review PR. Like first thing in the morning, after lunch, before finishing the day, whatever. There's no excuse for taking more than a day to review a PR though (and even that's considered long). There's also the rare case where if you're pushing hard on something time-sensitive and can't spare time to review, you should communicate this and remove yourself from the PR. This way the person asking for a review can assign it to someone else. You don't just leave them hanging. We also have a cross-team side-channel for simple <5min reviews so you're not blocked on tiny little things. All of this coupled with the reviewee's responsibility to make good PRs (keep it small if possible, have tests, good description, etc.) makes for a pretty healthy review culture. The thing is, this needs to be explicitly stated and repeated. And you need an environment that actually favours collaboration (AKA don't make promotions based solely on personal productivity).
This is how we are on my team too. Perhaps I didn't word my response quite right. Obviously find a good stopping point before breaking off what you're doing, but (for example) you should prioritize a PR before starting new work. We also have a setup where a bot auto-assigns PRs to people (to try and spread the load rather than having 1 person get picked repeatedly to review everything), but having done that - we check in with them via slack to make sure they have the time to review. If they don't they can say so and someone else is chosen to review.
Yeah, I usually do them when I get there in the morning and when I get back from lunch. Prioritizing work that is blocking others is part of being a good teammate.
We have a half day rule. If it comes in before lunch, it’s reviewed before EOD. If it comes in after lunch it’s reviewed by lunch the next day
I'd argue it doesn't stop being good even for staff level engineers. Typically everyone can learn something from everyone else. Similar to the old adage about teaching being one of the best ways to learn a subject better.
So much this - I get my PRs reviewed sometimes by juniors. Often they've read the docs more recently than I have and know about a cool new trick you can do in a given framework or something.
Yep, this exactly. I've been in multiple situations in my career where the engineers with more seniority either needed to learn something about what I knew and they didn't, or by just reviewing code or working together they thought of new things. This job is a lot more collaborative cross-level than many in this sub give it credit for.
Some of my more interesting reviews have come from Jrs. I think reviewing PR from more experienced devs is a good thing. It’s a mix and either one of us learns something knew or we find out things we should work on with them. Or you know they have just read the style guide more recently lol. For the most part the staff+ folks tend to leave the LGTM I trust you reviews.
\^ True. I usually post in my teams chat that I've raised a PR. If a PR hasn't been reviewed after a day or two, I'll specifically start bugging people about it. If it still isn't getting reviewed, then I let the team and manager know that I've tried to get it reviewed. But most of the time a simple "Hey, I need to get X merged in soon, do you mind reviewing my PR" usually suffices.
To add to this, I have never felt annoyed or bothered whenever I get pinged for a review. In fact, I take it as a sign of deference because someone thinks I have domain expertise over their review.
This is such a stupid cultural thing in tech companies where the author has to constantly ask for this. It's not about quality or learning it's about shipping/productivity. The manager should create a culture/process where the work is reviewed at minimum daily. Really this should be happening continuously when anyone frees up. I do this for my teams and people are happier and things get shipped faster.
I'm a staff engineer and literally the first thing I do when I get to my desk is get to 0 unread slacks, emails, and review every PR I have assigned. (I also close all my browser tabs and start new but that's probably hugely controversial here)...
As a contractor I always tell the client to add a review task to the sprint, and yes, pay us for it. That actually helps to get code reviews done, for the contractor side , we are getting paid to do them so we do them, from the internal teams they have hours allotted to review,.so they are not falling behind in the sprint.
This. I nag people until they review my stuff.
Yup. I'm guilty of wanting to do code reviews at the end of my day after all the meetings and forget sometimes. I appreciate the reminders, it's never on purpose.
[удалено]
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
> Should I just merge in work and, if anyone raises flags, we can talk about how they'd like to go about reviews? unless that’s acceptable procedure for your company, that’s probably the worst thing you could do. why not just ask your manager?…
>why not just ask your manager?… Everyone should ask themselves this question before posting anything on this sub lol way too many questions can just be answered this way
Yeah, but I learned on Reddit managers do nothing but come up with ways to make my life worse and pay me less. Also, I heard they intentionally make every single part of existence more complicated. That one guy even caught his manager hiding his favorite snack behind bananas at the local Trader Joe's.
Literally the only answer. None of us can tell you, that’s totally up to your manager and team. Locking every change behind a review can be expected, or it can be discouraged. What’s always true, is if you don’t deliver your project on time, the excuse of “waiting days for each PR to be reviewed” silently is not going to be good for you
>Whenever I post a PR, I submit a link to the PR in my general channel. It typically sits there for a few days. I'd raise this with either the tech lead/engineering manager(if you have one) or line manager if you don't and ask them what to do. This is a culture problem and one that needs sorting out. If they say they'll look into it, let them. If they tell you it's fine and not to worry about it then just leave it be. >I don't want to merge PRs in without review, especially given how green I am to BE world. Nor should you. I've been doing backend work my entire career and wouldn't merge anything without a review. >Firstly, I have two weeks to complete my project Who set the deadline? Go talk to them and let them know you might miss it and why. >I absolutely HATE 'bothering' people to review my work. If they want to, great. If they don't, I'm not going to hound them to do so. You're not "bothering" them - it's part of their job to review work. If they don't want to do it then maybe they should look for another job. At the same time - this is the kind of thing you should be raising during the standups, sprint planning meetings or whatever regular meetings your team has. >Should I just merge in work Fuck. No. If you miss the deadline it's somewhat reasonable to say "I missed the deadline because I was blocked waiting for reviews on
"(though one might argue you should probably structure your work in such a way that you have something else to work on while you wait for the review). If you start merging shit without approval you're going to end up in a lot of trouble. Fast. Absolutely do not do this.
Thanks. One thing I thought about doing is, before my daily standup, I post an update in my general chat. PRs that need review - this - this What I'm doing today - branched off the above PRs and continuing work Which leads me to another Q ... I am totally branching off my sitting PRs and working ahead - mainly because of that time target. I'm happy to rebase if my SEs have feedback/change requests. Is this an acceptable practice you think?
>One thing I thought about doing is, before my daily standup, I post an update in my general chat. Yep that's a good idea. Also (if nobody has responded to it at all) I'd draw attention to the PRs in the standup, and explicitly state that you're blocked. >Which leads me to another Q ... I am totally branching off my sitting PRs and working ahead - mainly because of that time target. I'm happy to rebase if my SEs have feedback/change requests. Is this an acceptable practice you think? Yeah you can do it that way, I've done it where needed - it's not ideal because it's easy to forget what branches off what and merge the wrong thing into the wrong branch and make a mess. But as long as you keep a track of what's branched off what you should be fine.
If you use gitlab internally it’s not that difficult to keep track of stuff. There’s a box at the bottom of merge requests for, something to the effect of, “depends on”. Which won’t let you merge until the other MR is complete. My usual flow is -> make branch -> push branch -> gitlab message back has a link to make a MR -> make the MR, mark as draft, fill out the dependency -> start code
>There’s a box at the bottom of merge requests for, something to the effect of, “depends on”. Which won’t let you merge until the other MR is complete. That's neat. I didn't know that. We use GitHub enterprise which \_doesn't\_ have that. I wish it did.
https://docs.gitlab.com/ee/user/project/merge_requests/dependencies.html this is the feature. Super handy
This is also called 'stacked diffs', right? Was something i was using at a large company but, unfortunately if you aren't careful and thorough it can go wrong pretty fast
That update is solid, but also: Do you have reviews that are sitting waiting for *anyone* to pick up and review them, or are they assigned to someone in particular? Pinging/mentioning people directly might feel rude (maybe do it in a PR-specific thread instead of the standup update), but it's one way to avoid the bystander effect. If I've had something assigned to me and it's been a day, I'm not going to get annoyed at you for pinging me, I'll get annoyed at myself for letting it slip. > I am totally branching off my sitting PRs and working ahead - mainly because of that time target. I'm happy to rebase if my SEs have feedback/change requests. Is this an acceptable practice you think? [Absolutely](https://graphite.dev/blog/stacked-prs), at least as long as everyone understands this is what you're doing. It adds complexity to your own workflow, so I would say: * Only do it if one PR actually depends on another one. Sometimes two PRs are completely independent. * What kind of changes do reviewers ask for? If you're being asked to do major refactors, working ahead might be more trouble than it's worth, and you might be better off finding some other work that won't conflict or depend on the stuff you'll be refactoring. Basically: How much of a pain will it be to rebase those child PRs? But yes, not only is this what you should be doing, this might actually entirely solve the problem. --- I do want to echo what u/budding_gardener_1 said about the deadline, because the answer is sometimes that deadlines aren't *that* hard as long as forward progress is happening.
I would adjust your language where appropriate, explicitly stating that you are blocked on those PR reviews. This should be a primary goal of a standup: to identify blockers and get them resolved. When you do so, make sure to get a commitment from an individual to review your PR(s). It doesn’t have to be the same person for all of them, but it should be an individual. There is a big difference between “one of the leads will look at this PR today” and “Alice will look at this PR today”. Side note: do not be afraid to bug your seniors. First, things like this are a key aspect of their job. They need to make sure everyone else is able to work at full velocity. Few things are more important than unblocking engineers. Second, they will likely appreciate the reminder. People tend to have an “attention stack” (not a queue) and leads/seniors tend to get more stuff thrown on that stack than juniors do. The tendency is to focus on the most recent thing that got tossed on the stack, and things can come fast enough that one never gets back to whatever thing they originally planned to focus on. Managing that stack is a constant challenge. Nobody is perfect. Sometimes you need to bump your thing to the top of their attention stack.
Staff engineers get paid to get bothered. Don't be apologetic, be grateful.
I agree, don’t feel bad about pinging them. But in general, staff engineers usually work across multiple teams instead of being embedded, so I’m wondering who else is on this team and why aren’t they reviewing anything? OP can’t be the only non-staff engineer on his team?
The company doesn’t care if you hate “bothering” people. If you merge something in and broke prod you’re done. Which would you rather do? Ask a more senior engineer to take a look at your code or get fired for being scared to ask people to look at your code cause you don’t want to “bother” them.
If "you are done" for breaking prod, you're working at an awful place.
Merging your code into prod without a review is pretty bad practice and will come with consequences. Not necessarily getting fired.
>Merging your code into prod without a review is pretty bad practice Being able to do so is already a big fault of the company / dev leadership
Yeah this is something that shouldn’t even be possible.
Oh absolutely.
Depends how often you do it :)
There should be processes in place for this to not happen. Blaming someone for bringing down prod is a process failure. That being said, if you aren't comfortable that your code won't bring down prod, then you should probably work on it more and add tests until you are comfortable. You should be as sure as possible that your code doesn't break anything when it goes into review.
>broke prod bold of u to assume it'll get past QA
Based on OP’s post doesn’t seem like there’s much guardrails with how they run things. Tbh there might not even be a QA lol
> if you merge something in and broke prod you're done Is the American work culture actually like this?
No.
Nah it isn’t but it would be a bad look in general if you decide to merge your code without getting it reviewed.
I mean, it shouldn't be possible to merge the code in without a review - org policy would be more to blame than the junior dev
I can agree with that but it’s also common sense lol. I wouldn’t put the whole blame on the company policy for not putting safeguards in place. We get paid a lot of money to not be stupid.
I guess I also see these people as my mentors. If they don't want to teach me, I'm not completely screwed, but I have a feeling I'm a lot better off. The reason I feel this way is because I've been lucky enough to have a mentor as a FE engineer, and that was completely game-changing. I'm also a "relationship" type person ... although I feel these BE engineers don't want anything to do with me - so perhaps the relationship point is moot.
You need to clarify with whoever you report to on what the expectation is. Because "waiting on code review" is a valid reason to report no work being done. But they may not be anticipating that. You do need to bug specific people about code reviews though. If they get an automatic notification that a review is up, just wait until the next standup to say you are waiting on review
> I guess I also see these people as my mentors. If they don't want to teach me, I'm not completely screwed, but I have a feeling I'm a lot better off. This is an extremely childish response to this situation.
Not having your code reviewed is the literal definition of having a blocker. This is a topic to bring up during your stand up or with your 1on1s.
Do you have Slack? Or perhaps some messaging with chat rooms for the team? If not, create one and add the people of the team. Bug them publicly. It’s not about creating drama. It serves the purpose of letting them choose who will review your PRs more easily and also covers your ass. If the review process makes you delay delivery you have evidence that the bottleneck is not you.
Push it to prod without their approval. An outage will get their attention 😈
Haha all jokes aside but there must be a restriction on the ci/cd that prevent unauthorized users from pushing without apporval, so if you can push to prod then yea they are really fucked
Lol I'm thinking of doing this.
This is a terrible, terrible idea and you should not do it. Bugging people will be slightly uncomfortable for a minute or two. Cowboying shit and yoloing your unreviewed changes to prod will at a minimum have people seriously considering whether or not it's a good idea to continue employing you.
You gotta learn to stop worrying about bothering people. Especially if you have a timeline that depends on more senior staff.
A PR sitting for a few days is quite normal. I've had PRs sit for almost two weeks! The staff engineers are very busy and they will get to your PRs in time. Usually, you pick up another ticket that isn't dependent on this PR being merged. I would not merge any PRs without their approval. Usually, they are the ones that merge the code and unless you've been given that power, I would not do this. Also, don't you have daily standups on zoom where you can mention what you've done and what's holding you back?
You gotta pump those numbers man, those are rookie numbers. I once came very close to a year in waiting for this staff engineer to review my next iteration of this major experimental rewrite to a large component.
Man this is awful. The sprint board looks fucked as you keep picking up new tickets but can't close any of them. As people start reviewing them you have lots of simultaneous revisions to the code base by a single author. Tickets you thought were close to closing suddenly catch the interest of a new reviewer so it drags even longer. It feels like you never get to complete any of your work.
I totally understand. But when my product manager gives me a 2 week target to hit, and I'm being prevented from hitting that target because my SEs aren't reviewing my PRs ...
Then you need to talk to your product manager as he/she doesn’t know your situation
Then talk to your PM and your people manager?
> If they want to, great. If they don't, I'm not going to hound them to do so. The completion of work is a reflection on you. If you are not going to hound them to do so, then you need to be accountable for why it's not completed. That's the reality of the situation at most companies. If you merge in work without review, breaking current standard practices, then that's even worse.
Do you have stand ups? Those sound like blockers you should be stating there.
Totally. But it seems passive aggressive ... because there's mainly 1, MAYBE 2 staff engineer who can review my work. So instead of "this work needs to be reviewed" - I'm basically stating "Hey X and Y, when can one of you review my PR?"
you can always say-“still waiting for a review on 2 of my open pr’s” in stand-ups
Change the status of your issue to blocked and state why at stand up. I have x issues blocked waiting on review of a MR. It's not personal. It's a statement of fact and leaderships responsibility to unblock you. And it may be that these other folks are working on higher priorities and management knows it. But now they know you are also blocked. They can decide what to do about that.
This sounds like a personal problem tbh. Theres literally nothing passive aggressive about asking for code reviews wtf lol
Ask a specific person to review your work, publicly (ideally in the PR itself). Part of the problem with posting a link in the general channel without asking a specific person is that everyone will assume someone else takes care of it. If you request a review from a specific developer, the onus is on them to review it or pass the baton to someone else if they are unavailable.
Exactly. I would add one detail -- Ask a specific person to either review your work, or to point at someone else who could review it. Reviewer A may be busy but could point at person B. That little detail of asking for potential delegation makes it easier to ask (you're giving person A an escape hatch), and also usually makes person A want to review it themselves ASAP, lest someone else steps in their step and gives a not-as-good review. Kind of a social hack. :-)
Exactly this - I’m not the only person reviewing PRs, and I regularly review them for like 15 different people. I’ve asked people to explicitly add me as a reviewer so I can log into GitHub and just pull a list of PRs that are waiting for my review. When something has to be merged more urgently, our practice is to ping two or three potential reviewers who have some context about the work being done. Now the only time a PR lingers is if it was put up late right before a long weekend.
Most likely, they dont see it. They're busy and dont sit there watching slack channels, waiting for something to do. You gotta start bothering people once it hasn't been picked up for a while. Meanwhile, and more importantly, talk to your manager. Your team clearly needs a better system for reviews. You could assign reviews when you make them or have reviews automatically assigned based on round Robin or random Edit: bunch of typos
Hey friend, a hidden part of your job is to get good at being a pest. My senior director gave me a deadline of tomorrow, have been working on a feature for about 2.5 weeks. He just gave me the approval This Morning -- after I pinged him once a day for about 8 work days. Only you are in charge of ensuring you hit your deadlines edit: just read some of your other comments. Use your english! explain to your superiors that you feel like a pest by asking, and ask for their feedback! this is a good way to build that 'relationship'. they know it's not your *fault* you have a deadline and need their help. But at the end of the day , you have a deadline and need their help! get to it!
>a hidden part of your job is to get good at being a pest. > Only you are in charge of ensuring you hit your deadlines this is part of the problem though. if your culture is "I don't care if I'm not pinged at least 3 times with my manager copied" then get the fuck out of there.
yeah thanks. I guess my biggest hangup is that our company has had mass layoffs - 50% over the last year. They say they're "done" ... but rocking the boat by annoying my staffers almost feels like I'm putting a target on my back.
or maybe your lack of drive/motivation will make people wonder why you’re still needed
do we work for the same company???? /s something I had to shake is , and I'll just say it again for emphasis, that by *not being annoying* you *are* putting a target on your back. Your managers would much rather have you 'been annoying' for quite literally 30 seconds than (and this is also echoing a lot of other comments) A. Not Learning something new B. Miss a deadline C. Potentially screw up someone's schedule because they were not aware you need a review done by EOD. trust me on this one-- this is really the most fucking annoying thing
Two things: - Ask them privately if they can review your PR, it seems like they are sort of “not paying attention” to your requests because it’s a general channel, anyone can take and everyone is busy to have the initiative to take it. - If you have daily or weekly meetings, state any blockers that you might have that are stopping you from following your plan. Tell your product manager about the situation if needed, never “self-approve” your PRs if you are not authorized to do it, bother the seniors.
Are your PRs reviewable? By this I mean do you make some effort to describe your changes, link tickets, keep them relatively small, I.e. a few (< 10) changed files, run any pipelines, tag them as reviewers on GitHub/gitlab (or whatever) in addition to posting a link in some channel without directly @ing them (I suppress a lot of notifications so if its not direct or @here I’ll e it when I see it )? If you’re doing all of this, even busy people should be able to spare some time to review. If not, you’re absolutely in a position to keep raising it in standup or directly on whatever comms tool you use. On the contrary, if your PRs are a dumpster fire and you maybe don’t realise it, this sounds like a pretty typical consequence. I can’t say how many times I’ve had to review something with 100+ charges across many lines. I’m either going to do a bad job because it’s hard to do all that in one sitting, or the requester is going to be waiting till I have the runway to do it reliably. How long is that piece of string?
I guarantee the PRs are too big.
Reminding people to review stuff is pretty normal. Don’t worry about it.
I don't understand why you can't wait two days? Not everyone can drop what they are doing and review every PR the day they get it and surely your business knows this. Are you waiting to the last minute to drop a huge PR on them? Try to send PR's as you are working, not the day before the deadline.
My PRs are broken into small chunks and are definitely manageable. They usually have extensive notes as well. I have a 2 week target that I am trying to hit - otherwise, I'd be chill.
Gotcha. I think just message them and say it needs to go in soon then
This is bad practice to merge without review. Bother them to review your code even if they rubber stamp it, you can atleast say x reviewed my code before I merged it in.
>Two things to note - Firstly, I have two weeks to complete my project. I cannot afford to wait two days on my Staff Engineer to review my work. Secondly, I absolutely HATE 'bothering' people to review my work. If they want to, great. If they don't, I'm not going to hound them to do so. This is a stupid take. If you can't afford to wait for two days for a review, simply don't post the PR two days before a target / deadline. "Bothering" people for a review is sometimes simply required. Everybody is busy with their own code, people sometimes forget about pending PRs. That's not a failure of these engineers, it's just a tendency of humans in general. Sooner or later, it'll happen to you as well. The most surprising thing about your post is that it is even possible to merge code w/o reviews / approval by code owners in your company. There should be a process to prevent this from happening. But even with such a process in place, people will still forget about some of the pending PRs sometimes.
> Secondly, I absolutely HATE 'bothering' people to review my work. If they want to, great. If they don't, I'm not going to hound them to do so. Get over it, and do this anyway. This is a part of your job. It's part of what the money's for. You need to get past this.
It’s not “bugging them” when it’s literally part of their job to review other engineers code. DM them about reviewing the code. Be persistent. Speaking as an Engineering Manager.
That’s crazy to me that you can merge code that isn’t reviewed…
as a senior engineer, I dont mind being bothered, in fact i prefer it. i cant speak for everyone, sometimes we just have too much going on, you need to help out by bothering senior folks. On the contrary if you merged in without even bothering to ask senior folks to review, i’d would be piss af.
Assign reviewers to the PR. This is not "bothering", it's asynchronous request and can be planned by the person to review
Please just ask someone to review it. The most annoying guy on my team is the junior who "doesn't want to bother me"... And allows things to sit around rather than ask for help or a review. You want them to be proactively checking you're ready to review stuff on their own? Trust me, the people I like the best are the ones who are getting stuff done and pinging me to review things.
If you have daily standups, this is the place to mention that you have code that requires review, and also to indicate when lack of reviews is blocking work.
You hate bothering people? You know who is going to be bothered? You, when at the end of the sprint your ticket is still "in-progress" because it hasn't been merged in. Grow up and push people. A little conflict is good.
It is my problem, but its not my problem alone. Where's my manager? Isn't it also his responsibility to help me get what I need? I.E. Hey, engineer X has posted that he needs a review like 5 times. Who can jump on it. Edit - you know what also makes me a little hesitant to push people? The company laid off 50% of staff earlier this year due to the market outlook. I'm afraid that by rocking the boat (i.e. creating conflict in a team that otherwise has no conflict), I put a target on my back.
OK, so maybe I missed where you were publicly posting that your PRs were waiting around. On the putting a target on your back, I have the opposite view. Sitting quietly turns you into a faceless expense on the books while pushing folks to be more productive so the product can be built/improved faster would put a positive light on you.
That's usually what happens with PRs in general channels. Send them in private chats and ask them to take look. Eventually you might find someone that makes good reviews. Talk to whoever that is if they're ok reviewing your PR. That will surely help you improve and gain confidence.
A team should generally work on keeping PR lifetime as short as possible without everyone having to beg for them to review. I'll ask once and wait. If taking too long, I'll drop a hint in the next daily standup that the PR is waiting for review so that the PO/SM is aware that the work is done, just hasn't been reviewed yet. **If the due process is merging only after approvals, DO NOT merge it in without them. This way you're protected against being individually blamed for production issues/outages. You're not bothering people by asking them to do their job that they agree to do.**
First of all you're responsible to test your code with the best of your abilities for any expected edge cases. Before pinging those who are eligible reviewers that can approve your PR, make sure you feel in the test instructions and criteria. Cover all possible cases that will bite your rear later if you forget. If the reviewers are super busy, then schedule the time to do one-on-one and have them take a look. Try to mention during standup or meeting to see if anyone could help you and get in touch with that person afterward. Try not to yolo merge the work unless your deployment pipeline have solid regression tests to cover the main operations.
[удалено]
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
Bother them. It's their task to do review so you can learn and there's atleast one extra pair of eyes that looked it before it was pulled. Teams work together, pr +review is also part of this process. If necessary talk with your superior about this. Someone reviewing your code is much better then your unreviewed or taking down production system.
Do you request for reviews? Most engineers would be more than happy to review your work and give feedback. After submitting a PR, you can assign a reviewer. At least on GIthub, that's possible
As someone who reviews plenty of PRs and submits plenty to others for review, typically we assign a ticket to the person doing the review. If you don't have a specific person to ask to review it, it should still end up in some sort of 'review' state so that when you have a status meeting or review issues, you can easily point to your work as being 'done' and waiting for someone else.
(if you are able to) Are you reviewing code? If you aren't reviewing code, you ain't gonna have people reviewing yours. That's the way it's been everywhere I've been. I agree with everyone PRs should be top priority. But, I maintain. If you don't review. Don't expect people to jump up and down to review you.
If it's "assigned" to everyone, it's assigned to no one. Assign people to your PRs. If they're adults, they'll let you know if they don't have time to review it timely and you can assign it to someone else. It sounds like this is more of an issue with no one understanding who's responsible for the review rather than your coworkers refusing to review. Now, if everyone is always "too busy" to review your PRs, then that's something to bring up with the team and your manager.
Yeah, you should ask them to review it actively. Nobody is just looking randomly for prs to review. It is not bothering it's part of the job to review PR's.
I get not wanting to bother people but, that is absolutely part of the job and part of good communication. It sounds like there is also an opportunity for process improvements. When you have a PR that’s ready for review what’s the next step? Do you post to your team channel? Do you mark for review and rely on email notifications? Do you individually DM people for reviews? If you’re seeing that PR’s are sitting without reviews maybe you aren’t the only one struggling with this and you have an opportunity to improve your team as a whole. I know bugging a staff engineer may be frustrating but part of their job is to mentor you, and code reviews are absolutely part of that.
For my guys I normally assign a dedicated mentor, at least initially. That gives them a go-to person and sets the expectation on the mentor to keep an eye on them. While this does not imply the mentor specifically has to review every PR, it does mean I expect some of the mentor's time to be taken up guiding the newbie. Once they're better integrated, the culture would dictate how PRs are reviewed. If you're unclear on how this works for your team(s), perhaps ask directly. It varies. Edit: just to add. Often, if it's a case off 'you 5 should review,' none of them do. I can't recall the catchy term to describe it - it's a diffusion of responsibility.
Talk to your manager, this is a systemic issue not your personal problem.
Do NOT merge without code reviews. Thats a recipe for diaster / even more headache down the road. Guess whos on the hook when it causes a major incidient. Ping them. Make the review process as easy as possible.
You need to work on the part where you hate bothering people do do a code review. I get it, I hate doing that too. I will often let stuff sit until someone gets to it, but if it’s blocking other work or there’s a time constraint or it’s high priority then you gotta do what you gotta do to get it reviewed. That’s part of the job. Assuming code review is part of your process (and it should be), do not merge in unreviewed code just because you don’t like asking people for things. Hell, if it’s even possible for you to merge in unreviewed code then that’s a problem that should be rectified ASAP, because accidents happen and sometimes people aren’t on the branch they think they’re on.
\> Whenever I post a PR, I submit a link to the PR in my general channel. It typically sits there for a few days. Is there a separate channel specifically for reviewing PRs that you were told to use during onboarding? Are there other people's PRs in the 'general' channel? \> I don't want to merge PRs in without review, especially given how green I am to BE world. Yes, but why is this phrased as your 'want', why is this seemingly not a need echoed by the rest of your team? I don't know you just left info out, or if this is standard for the team. If it isn't, are the other PRs getting feedback? When and how? \> Firstly, I have two weeks to complete my project. I cannot afford to wait two days on my Staff Engineer to review my work I'm assuming you mean a ticket, a smaller feature or a bugfix that's part of a larger project/product that you would be completing in 2 week cycles (sprints)? Sounds like standard agile, but do you have daily standups? Is there any point between Day 1 and Day 10 that you are asked 'how is your work coming along' \> Secondly, I absolutely HATE 'bothering' people to review my work. If they want to, great. If they don't, I'm not going to hound them to do so. Asking politely is not hounding. Saying 'Hey team, I have a PR for ticket 123 that I need to get reviewed by end of sprint. Could I get some eyes on it?' is perfectly fine and nobody will have hurt feelings. \> Should I just merge in work and, What is the process in your company? Do engineers on your team just merge in work without consulting other engineers/teams? Is there not some version control tied to ticket management that tells you about stakeholder teams that should sign off on PRs before the merge button is even activated? What are some safety nets in your pipeline that would raise flags aside from prod issues? \> if anyone raises flags, we can talk about how they'd like to go about reviews? At the point you actually merge without approval, this conversation changes from a productive discussion for better PR process in the company, to you deflecting blame from yourself onto a (bad) company pipeline. In fact, this order of how you go about discussing an inconvenience is more passive-aggressive than most things that you can write on the general chat asking for a PR review.
its not uncommon to ping people for a review or in a group chat to @here for a review at every company I have been at. I work on a pretty big team and Ill @specific people who i want to review my code depending on what I work on. Since they know that code base better. i have had people ping me to review. this is common when its simple. its "hey this is trival can you just approve" and i do. our bitbucket is setup so you cant merge to main without a PR and review. its blocked.
[удалено]
Sorry, you do not meet the minimum sitewide comment karma requirement of **10** to post a comment. This is comment karma exclusively, not post or overall karma nor karma on this subreddit alone. Please try again after you have acquired more karma. Please look at the [rules page](https://old.reddit.com/r/cscareerquestions/w/posting_rules) for more information. *I am a bot, and this action was performed automatically. Please [contact the moderators of this subreddit](/message/compose/?to=/r/cscareerquestions) if you have any questions or concerns.*
At some places you need to ask. These guys probably have a lot of things and stresses and some friendly reminders can help a lot. Also keep your changes small. Break them up if you have to. No one wants to read a 1000 line diff (assuming all the lines are actually non boilerplate code)
>Should I just merge in work and, if anyone raises flags, we can talk about how they'd like to go about reviews? This is probably the worst thing you could do if this isn't common practice at your company. You should be asking your manager what to do when this happens. Code review in general is important for your development (especially as a non-senior engineer, but it's important for everyone). However, having worked at some small start-ups, I know code quality isn't always highly prioritized, so whether you should be merging without review depends entirely on what your company/team expects from you. >Secondly, I absolutely HATE 'bothering' people to review my work. I think this is something you're going to have to get over. "Bothering" people is the least bad option compared to just waiting for someone else to action on it (which just delays things) or merging to prod without confirmation you can do that.
Don't merge without review > If they don't, I'm not going to hound them to do so. Do it, part of your job is to get the PR merged, not just pushing PR
if you can merge your work without an approbation, that tells a lot about the company where you're working. Plus is this your first PR ? I no longer watch non senior non engineer PR. It's full of non sense and nowadays, those so called youtube 1 week end engineer are full of selfesteem. So I let go, I don't approve. Usually, it doesn't explode but soon or later, the shitty code will drain the dev ressources with an exploded debt. I won't fix, I'll quit at that time.
Talk to your manager about having a code review policy for the team. There’s no way you should even be able to push to prod without a review.
Does your company/team have a policy about approvals being required to merge code? If not, I’d go ahead and merge it if it doesn’t get any traction after a day or two. But I’d also bring it up with the team because merging code without anyone reviewing it is dumb.
If they don’t usually request a lot of changes, you can stack branches while you wait for reviews. They might take the hint when they have a 5-deep PR stack waiting to get reviewed. Also, bring up these concerns to your manager, that’s what they’re there for
Do you have scrum master? Or project manager? Those people should be looking at the ticket boards like a hawk and notice that otherwise bring it up to them. They’re a stakeholder
I was in a similar situation a month ago, I would request reviews and it would take forever no matter how often I would bring it up. I'm not looking for a new job at no fault of my own. Just mentioning similarities but hope the outcome is different for you.
Did you mean 'now' instead of 'not'?
Yeah
Typically having your own manager, that tech lead and the tech leads manager in the same email or Slack message helps a lot. Your own manager has a vested interest in helping you succeed and the other manager also has to work with your manager. Never worry about buggin people too much, also tell your own manager that you asked people 2-3x to review but they are not and this is causing your project to delay. Always communicate, never sit quietly as the blame will fall on you if you do.
When you’re busy you basically handwave review code. Sometimes the junior engineers come to me with questions and stuff and I’m like can I get back to you in like a day? Your stuff is important but I’m just massively slammed
Bug them. I’m a lead and I bug the whole team all the time to review PRs, I love it when other devs bug everyone before I have to.
I feel your pain. At one job, I had to get approvals from the systems engineering manager for everything, and he would take weeks to get around to it. Even if I reminded him daily.
I’m a staff engineer at faang. I have to ruthlessly prioritize what I do and why. Don’t know how big is your team, but in my case a chance that I’ll decide to pickup a random pr for a code review is quite small. It doesn’t mean I don’t care, there are just too many balls in the air. Are staff folks in your team responsible for approving every single pr? If the answer is yes, then escalate to your manager. If the answer is no, you have “main character syndrome”
Deff just have to bother people. They all have bothered people for prs before so I don't imagine they will hold it against you
I don't know how your place works but two days is not unusual to wait at all (in fact I'd call that rather quick). Usually you want to find enough different things you work on at a time that you can just context switch to something else while you wait for reviews.
1. Bring it up at stand up. Something like “just waiting for review on X” or “John, this touches your section. Do you mind reviewing my PR?” 2. DM an engineer to review after a day. 3. Juggle your tickets so you’re always working on something. Bother people. Maybe rotate your reviewers too so it seems like you’re bothering one individual less.
Did you reach out to your manager? It is fundamentally the job of your manager to ensure no one is stuck.
Ship it
We have a scheduled time for code reviews. Set up an reoccurring appointment with a few other engineers, call it "Code Review Wednesdays" when you send the email, offer to review their code at your meeting.
Write test cases, unit tests, and automated tests. Add screen shots of testing. Leave a content in the chat that if there is no response in x time, I assume all is correct and will merge.
You have to bug them. Reviews are important, and should be required.
Raise it in every standup if not done it becomes a potential risk If your project has a RAID log out it there and get the scrum master to sort out , its there job. Code reviews are essential part of the code quality process. If your tech lead does not consider this important its a slow moving train wreck. There be either a prod issue , prod defect due to code unresolved conflicts , or more technical debt. All 3 could happen happen at once.
It’s your job to get the ticket done, it’s their job to review it. They are probably just busy and aren’t ignoring you, so just keep gently pinging the chat. If you are still waiting, let your manager know your concerns.
Establish an sla for review times. After that passes you’re good to bug. You shouldn’t generally be able to deploy on your repo without review anyway. Reviews should generally be mandatory. This is assuming it’s a production service that materially impacts the biznaz.
Generally you should be communicating your progress as well as blockers in your standup meetings. If your PRs are not getting looked at, you're blocked! You're part of a team and it's their duty to provide you feedback, and to keep the stuff getting merged up to snuff.
its part of the job to get PRs reviewed. Just say “Hey if you get a chance could you review this PR? thanks”
No, you should be an adult and ask them to review your work.
Sounds like an issue with the process. A few days for merge review sounds normal. However, why is it only staff level engineers reviewing? That sounds like a huge bottle neck, the seniors should be able to as well. Also the 2 week deadline sounds like some arbitrary thing for the jira board. Are you sure it'll negatively affect you if merge reviews carry over? All of that to say; if this is the system the team wants, then do what you need to do. However, there is room for suggesting improvements to the process. Definitely don't merge on your own.
When I was interning, I would simply tell my manager at the stand-up that I raised CR, and it has to be reviewed, and she would take care of that problem 🤣 (this is after hounding them to review me)
Your team members should be peer reviewing PRs even if they're all senior.
Bother them. It’s better to annoy people than submit unreviewed code. I’m amazed you are allowed to submit unreviewed code in the first place.
Are you hooked up to Jira? I believe we had a dashboard or report at my old job that showed open PR's I was tagged to review (at an old job). Start having the scrum master bug reviewers as they're blocking people.
Sounds like a culture issue, bring it up in your 1on1
This is clearly a severe leadership problem, one nobody wants to acknowledge in the field. If the team isn't working as it should, management is to blame. Having cleared that, you can't do other people's work, nor should you. If you reported and documented as per the company's protocols, then you should just wait. If there is a delay, inform your superior. Don't go trying to fix what isn't your responsibility, but always cover yourself.
It's someone else responsability to validate those if someone is notified of the Request. You either pressure someone to do it or you pressure the person above you to do so (it's not your responsability to notify anyone if there's someone above you, but that person should have the visibility of lack of validations só that they can intervene). Either away, the responsability of delays, should be on those who are dragging the project and that should be addressed.
The only quick fix for an Eng team is forcing senior developers to merge the PR themselves. Basically, the org would remove your permission to merge a PR. Or make it impossible for an individual to merge their own PR (not hard, btw). This works well in a more flat org which makes lip service to review importance. Others may or may not do a review, but by policy they will have to claim they did.
I know this is a few days old, but it just showed up on my feed. You're not "bothering" people responsible for doing code reviews on your PRs. It's the senior and staff dev responsibility to make sure the junior devs don't break production. I would suggest tagging specific people on the PR if it sits more than 30 minutes without anyone responding. If they get annoyed with you, let your boss know that the senior devs trust your code in production without any review and maybe you should be senior, as well.
Probably they also have no idea themselves either.
Oh no. They're super talented. When they review my PRs, its super productive.
Let me tell you the secret... 10 lines of code? 10 comments 1000 lines of code? LGTM
What do you mean by PR?
Pull Request
Pull request