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

61–70 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#61

Post by BowBun »

stack_framer wrote: post #55

My manager recently told our team that "AI usage" would be added to our engineering competencies, and we would all be expected to "use AI more." When I said my top preference for AI usage, by far, would be to eliminate human code reviews, the response was basically, "Oh, not like that."

That's a bummer! At my company we've started investing in what I'm calling 'semantic linting', which is basically running GPT over a PR with a set of rules that we iterate on. Already I'm finding huge value for style/pattern comments that linters can't easily catch, dropping warnings for common DB migration footguns, or notifying people of changing patterns/new ways of doing things. Been great so far!

Re: The Theatre of Pull Requests and Code Review

#62

Post by Entelligence25 »

stack_framer wrote: post #55

My manager recently told our team that "AI usage" would be added to our engineering competencies, and we would all be expected to "use AI more." When I said my top preference for AI usage, by far, would be to eliminate human code reviews, the response was basically, "Oh, not like that."

Rn 70% of ai is being used only for code review documentation purpose, i think we really need entire engg workflows to be ai automated for better team insights, better team performace , sprint assesment etc and track entire engg lifecycle.

Vision should not be AI code, but it should be AI beyond code.

Re: The Theatre of Pull Requests and Code Review

#63

Post by yunohn »

davedx wrote: post #41

It's a very common refrain but I don't really agree with it: "How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory." The problem with doing this is if you're building something a lot bigger and more complex than 500 lines o…

> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you

+100 to this. My job should be thoughtfully building the solution, not playing around with git rebase for hours.

Re: The Theatre of Pull Requests and Code Review

#65

Post by comprev »

pards wrote: post #7
jvanderbot wrote: post #5

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…

> and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.

Trust, but verify. We're only human after all :-)

At $DAY_JOB we need approvals from peers due to industry regulation.

Re: The Theatre of Pull Requests and Code Review

#66

Post by iamleppert »

I've tried his advice several times and it's been a complete failure. Splitting up a PR into a bunch of little PR's causes more problems than it solves, and it makes it 10x harder for the reviewer, no matter how much they complain about long PR's. Now they need to suss out some kind of ordering of the PR's, and navigate between multiple change sets for changes that depend on one another. It doesn't matter how well you can isolate the "concerns" a frontend depends on backend API, etc.

You end up creating more work for the reviewer, and most people just simply won't do the work of a proper review. You also don't have the advantage of any CI or tests running across the entire set of changes, so you also have separate CI reports to review. All this adds up for more places for bugs to hide or happen. All the same risks are still there, and you've also added a few more points of failure in the splitting process.

And for what? To end up, most likely merging in one PR after the next, for a feature that should just be all logically grouped together, or just squashing and merging the PR's together anyway.

Re: The Theatre of Pull Requests and Code Review

#68

Post by dijksterhuis »

xmcqdpt2 wrote: post #44
jvanderbot wrote: post #5

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…

> Minimally, I would like context for the change, …

… the why is important

> IMO many software developers … don't have explanations to provide. People not taking the time is what makes reviews performative.

… a lot of developers only consider the how.

i’ve had a lot of experiences of “once my PR is submitted that’s my work/ticket finished” kind of attitude.

i spent a year mentoring some people in a gaming community to become dev team members. one of the first things i said about PRs was — a new PR is just your first draft, there is still more work to do.

it helped these folks were brand spanking new to development and weren’t sullied by some lazy teacher somewhere.

Re: The Theatre of Pull Requests and Code Review

#70

Post by vinnymac »

jvanderbot wrote: post #5

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…

For any PR above a few line change, if a developer has not done a self review, I don’t review it all.

Instead I request that it is self reviewed with context added, prior to requesting re-review.

I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?”

9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy sometimes, even unintentionally so. We need better lint tools to help the monkey brain perform better. I wish git platforms offered more UX friendly ways to force this kind of compliance. You can kind of fake it with CI/CD but that’s not good enough imo.

Post reply on HN