PR Reviews

[u/El_Cato_Crande]

Been getting tasked with reviewing PRs more and more at work. Happy to have that responsibility and grow in that fashion. However, i'm also nervous about doing it properly.

Atm i have a process of sorts to work through things. Understand the change being made (filtering out a column value, combining datasets, manipulation etc). Approach to the change (drop nulls, join v union etc, creating an additional column from scratch or something). Execution of the change. Was it overly verbose and wasteful. Were certain downstream effects not properly accounted for etc

If it all looks good to me. Then I approve. Sometimes I might meet with the programmer to ask questions based on notes as I go through the PR. Other times I don't as it's not needed and I get things clearly.

Would like to know what approach some of you use? Why/how you it? Do you have different tactics for different situations?

Any advice/tips will be appreciated. Just looking to improve and get better at this process as I think it'll also help me get a better understanding of system design and large scale architecture which is something I want/need to get better at

[u/initcommit]

This question is kindof general and vague which makes it hard to answer. Some of the most basic questions about the PRs are:

"does it merge cleanly?"

"does it pass CI?"

From your descriptions it sounds like you're dealing with SQL code / stored procedures / DB migrations being committed to your repo? Are there more specific details you can provide as to what is causing the nerves?

In general if you have a good understanding of the systems that you are overseeing, and the tasks that your developers are working on, and workflows/best practices at your organization then you should have most of the context you need to review it and make the right decision to either merge it or request changes.

[u/El_Cato_Crande → u/initcommit]

sorry. merging cleanly and passing CI are part of the looking good part and checking this downstream.

it's PySpark. But yeah, spark/SQL it's pretty much that. Work as a DE for context

I guess the nerves come from worrying I miss something big picture wise which results in things breaking on merge. So wondering if there's checks or a process anyone has developed or used that they've found helps cover those things

I think I do. Part of why I ended up getting asked to help with them. Been getting more of them to do. So trying to streamline my process. Or maybe that'll come as I do more of them.

But I guess I'd like to know if people have standardized methods they use to review PRs that helps them go through one with peace of mind

[u/initcommit → u/El_Cato_Crande]

Well I mean, I think the main answer to your question is just having a test suite or testing process that you're truly confident in.

I mostly work with Java Spring Boot web applications, and before we do deployments we do a combination of running our automated end-to-end test suite in addition to some manual testing. The extend of the manual testing varies based on the scope of the change - the the bigger the change, the more thoroughly we manually test the functionality. We are working to build a more robust set of end to end tests that we're more confident in to reduce the need to manual stuff going forward.

Once in a while, despite our best efforts, we introduce some bug or issue. It happens, it's a part of software development. In those situations your ability to detect, troubleshoot, and resolve an issue is very valuable, and can make a good impression on those around you.

So I'd say that if you're feeling nervous, build out your CI / test suite / process to the point that it eases your nerves. If that doesn't do it on it's own, maybe some manual testing will. And if that doesn't do it, rely on your ability to fix something when it inevitably breaks, it won't be the end of the world.

[u/El_Cato_Crande → u/initcommit]

Really appreciate this

This job doesn't have the best culture of test suites that I can run and just be happy things are green. Would have to create them. Which is what I presently pseudo do manually with each PR I'm tasked with.

But I think your last paragraph sums it. It's impossible to never break anything. Idk if that dev exists. So I can't live in fear of that. But rather try my best with due diligence and trust myself to problem solve if things go poorly. Which I guess might be why I was asked to do this. I've stepped up and solved things a lot when things have broken.

Thanks for the kind words. I will try to create/write myself a guide to perform the reviews to help me have some peace of mind about the process

[u/Corpsiez → u/El_Cato_Crande]

Is it possible to execute those tests via a script or some other way with minimal/no input? Something that anyone on your team can execute easily and quickly.

If you can make that happen, it makes it much easier to change that same block of code in the future and have confidence that your change worked.

Starting a test suite isn't a walk in the park, and it's downright daunting when the codebase is large. But it becomes more and more important as the codebase grows and time goes by.

If it's strictly a culture thing, talk with the people in charge and your team in general. Think about installing rules that every software change has to be accompanied by tests that verify the changed functionality. It doesn't need to be anything major to start.

[u/initcommit → u/Corpsiez]

With AI it is easier than ever to at least plan a direction to go in. Definitely not to slop it up, but with good spec and direction it can shoulder a lot of the heavy lifting esp for a single developer spearheading the effort.

[u/initcommit → u/El_Cato_Crande]

Of course! Happy to help. And yes it took me a while to realize that if something about the culture isn't working, sometimes it is best to take the initiative to implement the thing that will make your life easier and hopefully that mindset might spread.

[u/Few_Intention7941 → u/initcommit]

you already got the right mindset by actually reading through the changes not just skimming for syntax errors

my process is similar but i also keep an eye for test coverage, if they added new logic but zero tests then i ask questions before approve

[u/xao_spaces]

Thanks for asking this question, I’m a junior and I’ve been tasked with the same.

[u/OkOcelot6725]

For data pipeline PRs, a short checklist of the things that break quietly on merge helps more than a general process:- Joins: could the join key be duplicated or null on either side? Row counts before and after should match what you expect. A fan-out join is the classic silent bug.- Schema: any column renamed, dropped or changed type that a downstream job reads?- Reruns: if the job runs twice, or backfills old dates, does it overwrite cleanly or double-count?- Filters: what happens to rows the new filter drops, and does anyone downstream rely on them?- Evidence: ask the author to paste a before/after row count or a small sample diff in the PR description. If they can't, that's a useful signal in itself.Keep it as a PR template so the author fills it in before you review. Then your attention goes to the judgment calls instead of rediscovering the list each time.For data pipeline PRs, a short checklist of the things that break quietly on merge helps more than a general process:

- Joins: could the join key be duplicated or null on either side? Row counts before and after should match what you expect. A fan-out join is the classic silent bug.

- Schema: any column renamed, dropped or changed type that a downstream job reads?

- Reruns: if the job runs twice, or backfills old dates, does it overwrite cleanly or double-count?

- Filters: what happens to rows the new filter drops, and does anyone downstream rely on them?

- Evidence: ask the author to paste a before/after row count or a small sample diff in the PR description. If they can't, that's a useful signal in itself.

Keep it as a PR template so th

[u/JohnVonachen]

Make a checklist and go through it for every page or whatever. Do you have some kind of static analysis? you can have a config file that checks for each thing on your list. You can make an analysis tool that will use regular expressions to help you. You can tweek it as you go.

[u/iOWIIPZJ51ucF2H]

my rule of thumb is the 2-comment rule: if a pr comment has more than two back-and-forths, stop typing and grab a 5-min huddle. text debates on prs waste hours, while code comments should strictly be for quick nits or obvious bugs.

[u/omegaonion]

LGTM

[u/wggn]

it's never a bad idea to ask the engineer for a code walkthrough for bigger changes.