Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

Post reply on HN
Print view View original

171–180 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#171

Post by BobbyJo »

MoreQARespect wrote: post #162
herval wrote: 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…

The "mid levels who consider themselves senior" are the exact type of people who I see saying what you're saying, i.e. * Yes, TDD on production code is nice in theory, but it doesnt work in my case . * Yes, short PRs are nice in theory, but it doesnt work in my case . In every case, as far as I can see, it meant "It does work, I just dont know how to do it". When I say "if you dont think it works in your case, come t…

> Yes, TDD on production code is nice in theory, but it doesnt work in my case...

Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided".

I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.

Re: The Theatre of Pull Requests and Code Review

#172

Post by goosejuice »

eCa wrote: post #166
goosejuice wrote: post #109

Earlier quoted context omitted.

In my experience, US healthcare, that box can be checked at later stages, namely deployment to production. It's a choice to add it earlier.

If it is for checking a box, sure. If it is part of a process that aspires to deliver projects with quality and with somewhat predictable release dates, that seems way too late, imho.

Sadly it often is box checking, code review or not. I'm only stating that there is no requirement in US healthcare that I'm aware of that requires approvals before merging code. Maybe that's not true in other industries. But most regulatory frameworks that I'm aware of are flexible, ambiguous, on implementation details by design.

If you find that outcomes are the same by making approvals optional at that stage, then do so with accompanied justification.

Re: The Theatre of Pull Requests and Code Review

#173

Post by t-writescode »

eCa wrote: post #166
goosejuice wrote: post #109

Earlier quoted context omitted.

In my experience, US healthcare, that box can be checked at later stages, namely deployment to production. It's a choice to add it earlier.

If it is for checking a box, sure. If it is part of a process that aspires to deliver projects with quality and with somewhat predictable release dates, that seems way too late, imho.

And a great way to end up leaking customer data from a SQL injection or other error that could have easily been caught during a more piece-wise analysis and vetting of the related code nearer to time of writing.

Re: The Theatre of Pull Requests and Code Review

#174

Post by crazygringo »

I can't agree with a lot of this.

PR's should generally be the size of a feature, or a meaningful subfeature for large features.

When you arbitrarily split up PR's into something "300 lines" or "5-10 minutes" you can miss the forest for the trees. The little thing looks fine in isolation but doesn't make any sense as part of a larger approach. Different people are reviewing it piecemeal but nobody is reviewing the approach as a whole or making sure the parts fit together right.

And then the idea of "telling a story with commits" feels like a waste of time to me. I have no interest in the order in which you wrote the code, or what you wrote and then rewrote. The code itself needs to be legible. Your final code with its comments should speak for itself. Code is the what and comments are the why.

Now, what I will say is that the more junior the developer, the smaller their commits should be. But that's because they should be assigned smaller features, and have more handholding along the way. And when people are making larger architectural changes, they should be getting signoff on their overall approach from the start -- if you're frequently rejecting the whole approach to a problem in code review, something's going wrong with your communications processes.

Re: The Theatre of Pull Requests and Code Review

#175

Post by BobbyJo »

leetrout wrote: post #157
herval wrote: 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…

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"

Re: The Theatre of Pull Requests and Code Review

#176

Post by sexyman48 »

mystifyingpoi wrote: post #164
sexyman48 wrote: post #104

Earlier quoted context omitted.

You might as well go into HR. Everyone knows reviewing other people's PRs is like nurturing their kids at the expense of your own.

Well, then just don't play the game. Make a decision in the team, that everyone accepts everyone's PR immediately without any review. At least you won't have to wait.

I'm semi-retired now, but I spent most of my career at a Bell Labs-caliber place (I was the dumbest person there) before "PR" and "code review" became part of the lexicon, and yes, everyone was good enough not to mess things up too badly.

Re: The Theatre of Pull Requests and Code Review

#177

Post by mountainriver »

herval wrote: 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…

Thank you, more people need to read this. The software industry seems packed with these strange gatekeeping structures that only hinder development.

Focus on customer outcomes, and keep main clean.

Re: The Theatre of Pull Requests and Code Review

#178

Post by jeffbee »

goosejuice wrote: post #103
tcoff91 wrote: post #92

Earlier quoted context omitted.

By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.

I thought everyone did this. I review twice. For each commit with -v and finally in GH/GL after I open the PR/MR. I often catch something on that last one. It's rubber ducking.

Unfortunately, these days, I am getting a lot of PRs where nobody has read the code, which came straight out of a robot. This makes me really angry.

Re: The Theatre of Pull Requests and Code Review

#179

Post by roguecoder »

KronisLV wrote: post #137

> This makes logical sense, but it's challenging to implement because it can feel like admitting we're not smart enough to understand the code. However, saying "I don't understand this enough to approve it" is far more valuable than pretending with an empty "LGTM". Sounds nice but I’m sure that there are projects out there that are like constantly being in the trenches, testing in prod and the original developers bei…

If that's what the code is like and you aren't allowed to make it better, that is not a safe workplace.

Especially if the software matters at all.

I purposefully find jobs in fields like healthcare and physics and finance where it actually matters that the software works. Right now, if there is a bug in my code people could die.

And in that case, there are worse things than being fired.

If people do find themselves in that situation, the best answer is "unionize". The second best answer is to work with your coworkers to adopt better practices (it is very unlikely that they are going to fire you all all at once). And the third best answer is to do the job well, regardless of what is going on around you, and if you get fired you get fired.

Re: The Theatre of Pull Requests and Code Review

#180

Post by MoreQARespect »

BobbyJo wrote: post #171
MoreQARespect wrote: post #162

Earlier quoted context omitted.

The "mid levels who consider themselves senior" are the exact type of people who I see saying what you're saying, i.e. * Yes, TDD on production code is nice in theory, but it doesnt work in my case . * Yes, short PRs are nice in theory, but it doesnt work in my case . In every case, as far as I can see, it meant "It does work, I just dont know how to do it". When I say "if you dont think it works in your case, come t…

> Yes, TDD on production code is nice in theory, but it doesnt work in my case... Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided". I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.

Give me one example then. One is all it takes to disprove a rule.

Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example?

The only exception i can think of is something where 99.9% of the changes are autogenerated (where i wouldnt really be reading it carefully anyway, so the length is immaterial...).

Post reply on HN