Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

181–190 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#181

If your goal is to lower the velocity of your organization, e.g. because in practice code churn or poor quality are major problems, then by all means do this. If you still need to move fast, then don't. This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you…

Changing cowboy code is much, much slower than changing good code.

If you need to move fast for the next two weeks, sure. If you need to move fast for the next year, you are better off collaborating.

Re: The Theatre of Pull Requests and Code Review

#182

Earlier quoted context omitted.

My simple suggestion to my teams: PRs are emails to your team and to your future self. Framed in that context it's easier to carry the correct tone and think about scoping / what's important. --- > pedantically apply DRY to every situation I swear DRY has done more damage to the software industry from the developer side than it has done good because it has manifested into this big stick with which to bludgeon people…

A great way to frame DRY that I heard from hackernews: "DRY things that are supposed to have the same behavior, not things that happen to have the same behavior"

This is a really good way to put this. The "just because the do they same thing right now doesn't mean they _do the same thing_" concept is hard to convey!

Re: The Theatre of Pull Requests and Code Review

#183

Pair programming > PR reviews. I've heard people scoff at the $$ cost of "mob programming". I think that view is totally myopic, for appropriate problems there's just no faster nor higher bandwidth way to transfer code knowledge in a group. Plenty of people dislike pair programming, i don't dislike it but i do find it mentally intense, tiring. I really really enjoy that it's an accelerator for getting to done - not j…

The only time I like pair programming is when I (or someone else) is stuck on an issue and we need to work together and bounce ideas off each other; otherwise I prefer to work on a problem myself rather than trying to be in constant contact with me paired person.

Re: The Theatre of Pull Requests and Code Review

#184

Unless you have a broader context, reviewing 300 line PRs in 5 minutes is going to be surface level at best. Plus that time comes with an expensive context switch so the actual cost is likely more like 20 - 30 minutes. At this point, I think a reasonable question is why not just use AI for shallow reviews like this? This would free up bandwidth in situations where you really want code reviewed by someone else.

One of the advantages of small, frequent code reviews is that a team shares the broader context.

Which is far more valuable than ten minutes of extra typing.

Re: The Theatre of Pull Requests and Code Review

#185
It's a nice thought, but PRs, when more than performative, are often more about basic sanity checks than anything else.

There are two things that can be involved when a change is submitted: a design change and an implementation change.

Design changes that are significant enough to warrant review should be expressed and discussed in text, not code, in design documents of some kind. These tend to be quite general, but they establish agreement about general structure, which tends to be the most difficult to change later on, as it circumscribes and guides the implementation that follows. Writing is also a way of working out ideas. The very act of having to explain something clearly to someone else forces you to confront your own ignorance and the consequences of your proposal. Besides, it's the job of the person proposing the design to work out those consequences so that others can verify whether they're true. (1)

Reviewing implementation changes with an understanding of design allows the reviewer to understand how changes relate to the whole as well as the aim. This is an insider's perspective. (2)

Reviewing implementation changes without a good understanding of the context will be limited to general technical remarks, but can descend into excess attention given to style or inconsequential matters of taste. This is the outsider's view. (3)

The question, I think, that looms in the background is whether familiarizing yourself with the context sufficiently well so that you can judge the PR submission is reasonable for a PR. In many cases, it isn't. It's too time consuming and the context is too big. If we had infinite time, this would be great: having to explain to an outsider what you've done forces you to give a much more thorough account, if your goal is to achieve thorough understanding. It also exposes your thinking to someone who doesn't necessarily share your assumptions. But this can be a Herculean task for anything sufficiently complex. So the criticality of the change must be weighed against the effort needed to explain or learn. Are you making a change to something with high tolerance for error, or a small margin of error?

Two pieces of advice...

Since it is unrealistic to expect an exhaustive verification all the time, focusing more on tests will be more fruitful. You still need context to judge whether they're exhaustive or test the right things, but it's the one place where correctness criteria are expressed in code, apart from type signatures, in a clear enough manner that expectations can be judged. If they aren't clear, you should ask. It's a good locus for discussion.

The second: code doesn't include the rationale or "why" for what it is. It just is. Context goes a long way to help infer reason for the change. This means we should use comments, either in the code or the PR submission itself, to explain changes. If something isn't sufficiently clear, ask.

But the key is prudential judgement. You have to determine how to limit your verification efforts and when to begin accepting on trust for practical reasons.

And do away with your pride. It's only difficult to ask questions if you suffer from pride, and pride is a sure sign of mediocrity. You're also lying through omission.

Re: The Theatre of Pull Requests and Code Review

#186
post #150

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…

>nobody reads intermediate commit messages one by one on a PR, period.

>Don’t waste your time writing stuff for no one.

I've thought about that as I continue to write them. I think I can justify it by saying they are mostly for me. Can I describe what I'm trying to do with a specific push into a few items. It let's me reflect if I'm waiting too long between commits or if my ideas are getting too spread apart and really should be in two different branches that each have their own PRs. Then there is the rare case on a slower project where an item gets deprioritized and I come back to it weeks or even months later. Having the messages help me catch back up to speed.

As such, I find the 20 seconds or so to type out 1 to 2 sentences to be worthwhile, even if the ones reviewing the eventual PR never check. I'm also not above throwing in a "ditto" or "fixed issue" when a single commit really is that small or insignificant.

>“every commit must compile”

I agree with your take this is overzealous, but to expand upon my previous point, if I know a commit on a branch won't compile (say just had something else come up and need to swap focus for a few days), then I'll try to make sure I call that out in my last message just in case anyone else happens to get put on the project.

If I were to summarize my approach, treat PR messages seriously, but treat branch commit messages like sticky notes that will likely end up in the trash by week's end.

Re: The Theatre of Pull Requests and Code Review

#187
post #150

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…

> “nobody reads intermediate commit messages one by one on a PR”

I clean my history so that intermediate commits make sense. Nobody reads these messages in a pull request, but when I run git blame on a bug six months later I want the commit message to tell me something other than "stopping for lunch".

> pedantically apply DRY to every situation or forcing others to TDD basic app

Sure, pedantically doing or forcing anything is bad, but in my experience, copy-paste coding with long methods and a lack of good testing is a far more common problem.

You may be 100% correct in your particular case, but in general if senior devs are complaining that your code is sloppy and under-tested, maybe they aren't just being pedantic.

Re: The Theatre of Pull Requests and Code Review

#188

Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices. I trust my colleagues to do the same (and they often do). I can't imagine working in an environment where this is a theater.

A lot of developers are working in hellish companies, micromanaged and beaten down into cogs.

One very valuable skill for those of us who have experienced productive collaboration is learning how to introduce it to new places.

Sometimes that means telling the executives that their promotion process is making them less successful. Sometimes it means wandering around PRs leaving useful positive comments to seed the idea that PRs can be useful. Sometimes it means pointing out tests only when there is a bug, so people can experience how great it is to follow the practices that keep us from introducing bugs in the first place.

I wish that more CS programs would explicitly teach their students critique skills, the way art and music and english and math programs do. But until then, we're counting on engineers getting lucky and landing in a functional workplace like yours.

Re: The Theatre of Pull Requests and Code Review

#190

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…

This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.) Minimally, I would like context for the change, wh…

My team uses a github PR template with the following sections. Answers to each can be short yet it has been extraordinarily helpful to pass over important info to the reviewer that's not captured in code. It also borders on "checklist" that the code author has actually done the bare minimum to think things through.

# Goal (why is this change needed at all)

# What I changed and why I did it this way

# What I'm not doing here and how I'll follow up

# How I know it works (optional section, I include this only for teams with lots of very junior engs: "added a test" is often sufficient)

Post reply on HN