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

191–200 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#191

Post by TiredGuy »

roblh wrote: post #146

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files hav…

I agree this can be a bit of a pain if you're used to that. There are ways to partially reduce it: 1. use the timeline panel for an individual file to see all the historical changes to a file, and you can highlight multiple ones to see cumulative changes, and you can filter to only git commits or only local changes etc. 2. use the commit history panel in the source control area to do the same across commits, but it doesn't allow you to highlight across commits for cumulative changes

It does require a bit of a paradigm shift sometimes to not rely as much on seeing all cumulative changes for the ticket highlighted as you code, and instead compartmentalize your immediate view to the current commit's task, but often the above 2 alternatives help suffice. Of course, you did mention that you'll commit stuff you're not likely to touch again, which helps a lot too

Re: The Theatre of Pull Requests and Code Review

#192

Post by 1dom »

MoreQARespect wrote: post #180
BobbyJo wrote: post #171

Earlier quoted context omitted.

> 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...).

I agreed with you initially.

> I'm 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?

To me, when I meet experts in any field, the quality that stands out isn't that they do everything to expert level, it's that they get everything done as they said they would. Sometimes that means big PRs, because that's the environment created, and the expert finds the way to get the job done.

I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality, rather than just doing something because it's best practice and expecting everyone else to do the same.

They ultimately get the thing done how they said they would.

Re: The Theatre of Pull Requests and Code Review

#193

Post by anal_reactor »

My biggest problem with reviews is that I just don't care. I work for a company that does nothing interesting with a coworker that doesn't like me under a manager that is incompetent. I have zero reasons to do a good job, and all the reasons to do bare minimum not to get fired.

Re: The Theatre of Pull Requests and Code Review

#194

Post by tossandthrow »

DavidWoof wrote: post #187
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…

> “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 an…

> 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.

This is a false dichotomy and an unproductive thing to focus at.

Experienced engineers know when to make an abstraction and to not. It is based in the knowledge about project.

Abstarct well and don't do compression. Easy said, and good engineers know how to do it.

Re: The Theatre of Pull Requests and Code Review

#195

Post by Jcowell »

ed_blackburn wrote: post #72
williamcotton wrote: post #67

Pro tip: Get your potential code reviewers involved before you even start coding. Keep them abreast of the implementation. When it comes time for a review they should be acquainted with your work.

This. Absolutely this. The PR should be the final step in the process. Never the first. Ambushing people with PRs and then demanding their attention is a massive time and mood sink. It is ineffective and counter-productive. You may as well just commit to main, and honestly, in so many situations I think that's perfectly rational thing to do, so much about PR culture is theatre.

This seems like a bit of an over exaggeration. A pull request isn’t just a chunk of code thrown at people. it’s an entire process with a Title , description, pipeline checks that all come together to say I want this in another branch.

As the PR author it should be your job to: * Self Review the PR * Ensure you adhere/fulfill to all the expectations and requirements a PR should have before it’s pulled out of draft * That all pipeline test pass * That for a given request X there is test that validate X, Not X , and edge cases of X and are ran in the pipeline. * Has a clear description of what you’re changing/adding/removing, why, how, and the rollout plan , roll back plan , & the risk level.

The peer review process should make the reviewer engage in a rubber duck process to review their code , loop the team in for changes that can change their mental model of how a system they own works, and to catch things that we might not catch ourselves.

Not to the mention the security implications

Re: The Theatre of Pull Requests and Code Review

#196

Post by conradludgate »

roblh wrote: post #146

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files hav…

My recommendation: don't treat the development process as the final draft.

Write everything in one go, and then afterwards rework the commits to tell the story you want to tell. This does require getting very comfortable with git rebase, but I can absolutely recommend it.

Using this technique, I've managed to merge several large refactors lately with no issues, and the reviewers didn't hate me.

Re: The Theatre of Pull Requests and Code Review

#197

Post by mtlynch »

g-b-r wrote: post #141
mtlynch wrote: post #46

I love code reviews and blog posts about them, but I vehemently disagree with all of this advice. > His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits. > While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story. This is ridiculous! You absolutely can and should review a PR without demanding…

> Intermediate commits are for the author's benefit, not the reviewer's. I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done. Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of…

>I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.

Intermediate commits are just checkpoints of unfinished code. The author knows that they made them and can revert back to them or use git log --pickaxe-S if there's code they saved to a checkpoint and want to recover.

Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author.

It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people. If I write a post-it on my desk that says "beef," that can remind me I need to pick up beef from the store, even though if my co-worker reads it, they wouldn't know what the note is trying to communicate.

>PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.

I don't understand this expectation that an author produce this.

What if the author experimented with a lot of approaches that turned out to be dead ends? Is it a good use of the reviewer's time to review all the failed attempts? Or is the author supposed to throw those away and reconstruct an imaginary commit history that looks like their clean, tidy thought process?

If someone sends me a short story they wrote and asks for feedback, I can give them feedback without also demanding to see every prior draft and an explanation for every change they made until it reached the version I'm reviewing.

The unit of change for a code review is a PR. The intermediate commits don't matter to the reviewer. The unit of change for a short story is the story. The previous drafts don't matter to the reader.

Re: The Theatre of Pull Requests and Code Review

#198

Post by mnembrini »

roblh wrote: post #146

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files hav…

In Intellij if you open a PR it will highlight lines that have changed in the PR differently (even from multiple commits) and you can click on the colored border to see the version of the code on the main branch

Re: The Theatre of Pull Requests and Code Review

#199

Post by chrislo »

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…

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

I do! I find it the easiest way to review code when the author has taken the time to structure it in that way. I'm lucky to work with some great people.

Re: The Theatre of Pull Requests and Code Review

#200

Post by jonahx »

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…

> nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one.

I do. Especially if the author is competent.

That said, empirically, you're correct most people don't.

However, that said, I think changing the culture rather than throwing away the practice would be a better response.

Reading and reviewing clean history is really so much nicer. I'd also argue that actually making your history clean (as opposed to theatrically and thoughtlessly making small commits, say) forces you as the author to review it more carefully.

Post reply on HN