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

291–300 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#291

Post by kiitos »

markus_zhang wrote: post #239
roguecoder wrote: post #205

Earlier quoted context omitted.

"Why is the specific id excluded?" is exactly the kind of thing we can capture in code, and that a good code review will flag. It takes two seconds to write `EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY=[1] [...] NOT IN EVIL_IDS_THAT_STOLE_OUR_LUNCH_MONEY` instead of `NOT IN [1]` and then not hate your past self six months from now when you have to figure out why something has been excluded. If some code can't be understood t…

> We absolutely expect our reviewers to fully understand the code. I suspect you don’t work as a DE or your company has top notch culture. I have never worked in any companies (including some big ones) that encourage a full understanding of any code submitted to PR review.

more or less the entire point of code review is to ensure that the reviewer fully understands the code that they review

i know that lots of orgs don't do it this way, but it's really important to make this point very clear: those orgs are wrong, and pathological, no matter how many of them there are

Re: The Theatre of Pull Requests and Code Review

#292

Post by sodapopcan »

kiitos wrote: post #267
sodapopcan wrote: post #251

Earlier quoted context omitted.

> Reading and reviewing clean history is really so much nicer. You can have both with git and it's not even hard. Unfortunately it seems many people pride themselves in what little they know of git. I'm not being sarcastic, I've read people say this almost word-for-word.

git is a means, not an end commits mean precisely what their author intend them to mean, nothing more if you squash-merge every PR then history is clean where it matters

To quote my least favourite HN response: "No."

Re: The Theatre of Pull Requests and Code Review

#293

Post by kiitos »

gjgtcbkj wrote: post #51
surgical_fire wrote: post #16

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.

It sounds like a good job where the most important part is finding other people’s mistakes. Though I do appreciate the shoutout to adding tests in CR. But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. It kind of like publicly executing someone, theirs gotta be some performance for it to be a deterrent. If something doesn’t have tests my review is going to be a very…

i think you would benefit from some time in an organization that was not shitty

Re: The Theatre of Pull Requests and Code Review

#294

Post by bob1029 »

1-more wrote: post #288
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. Very common practice at my old company, and one I continue in my current role. > “every commit must compile” sucks ass for anyone else trying to rebase your branch onto the update main/master when they don't. Once your PR is out of "working on the feature" and into the "getting it merged" phase, do a little `git rebase -i` and squash your really…

> Ignore this if you have real grown up CI where your PRs never stay open for more than a day.

A vast majority of the drama that comes out of source control is associated with branches living for far too long.

I've got an internal alarm that starts to go off somewhere around 72 hours. If something takes longer than this, I've probably screwed up in my planning phase. There are some things that do need to sit, but they should be rebased every morning like clockwork. The moment things start to conflict, the PR gets closed and the branch is now a reference for how to do it again when whatever blocker is cleared.

Another way to think about all of this is to pretend like everything you are touching is taking a synchronous lock out (even if it's not), similar to how tools like Perforce behave. So, you generally want to move as quickly as possible to get out from under lock contention. Git allows you to pretend like you aren't conflicting for a really long time, but at some point you must answer for all of this debt (with interest).

Re: The Theatre of Pull Requests and Code Review

#295

Post by viraptor »

eigencoder wrote: post #165
viraptor wrote: post #22

Earlier quoted context omitted.

> only you hide the way all changes interact with each Splitting the change does not prevent you from looking at diffs of any combination of those commits. (Including whole pr) You're not losing anything. > at least 10x longer because an average approval time is often more than a day. Why do you think it would take longer to review? Got any evidence?

I think approval time would take much longer. The issue is that while actual time spent in review may be shorter, there's a lot of context-switching time costs that increase with the number of PRs submitted. No one on the team is just sitting there refreshing the list of PRs, ready to pick one up immediately. There's a delay between when the PR is marked as ready and when someone can actually get to it. Everyone is t…

I don't think that's a realistic view of the timeline. I've done features as multiple PRs and there are really two cases:

1. I can't submit pieces until I have the final version. PRs go up at the same time and can be reviewed one after another immediately.

2. There's a very specific split that makes that feature two features in reality. Like adding a plugin system and the first plugin. Then the first part gets submitted while I still work on the second part and there's no delay on my side, because I'm still developing anyway.

Basically, I've never seen the "but if each PR has a delay of 1 hour before a reviewer can get to it," getting serialised in practice. It's still either one time or happening in the background.

Re: The Theatre of Pull Requests and Code Review

#296

Post by kiitos »

g-b-r wrote: post #216
mtlynch wrote: post #197

Earlier quoted context omitted.

> 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 mes…

> 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. After a few months it will probably be as useful to you as to anyone else; if you only use commits as some sort of help while developing…

> if you only use commits as some sort of help while developing, you might as well just squash them before making a PR.

yeah for sure you want to squash-merge every PR to main, right?

commits are just commits, there is no moral value to them, there is no "good history" or "bad history" of them, whether or not they're "made thoughtfully" isn't really interesting or relevant

git is just a tool, and commits are just a means to an end

Re: The Theatre of Pull Requests and Code Review

#297

Post by conradludgate »

kiitos wrote: post #282
conradludgate wrote: post #196

Earlier quoted context omitted.

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.

holy moly absolutely not git is a means, not an end code review is about the code as a unit whole, not the steps along the way!

Hard disagree. I've found this way of working benefits both me and my colleagues.

I'm able to work on long lived branches without getting into conflict hell because when I rebase, I'm only dealing with small conflicts.

I'm able to slim down initially large changes into smaller total diffs because I realised one change wasn't actually necessary, but only because I took the time to reflect on the code and separate the concerns

Being able to separate your code into smaller units is a really great tool, and helps you really understand your own code changes in a new light. Amusingly despite me often rewriting the same code 3 times, I feel like I've never been more productive (and no, I don't use any LLMs)

Re: The Theatre of Pull Requests and Code Review

#298

Post by mateo411 »

lamontcg wrote: post #257
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…

Sometimes you're overhauling something which you can't do in chunks less than something 2,000 line long PR. There's no intermediate working system. The problem is, "take this very large bit of code and throw it entirely away and rebuild it completely differently". Trying to craft some evolutionary step between A and B is just going to take 10x longer and won't help any code reviewers.

I agree.

When you have a large PR like this, here's how I like to get it reviewed.

1. Give reviewers sometime to become familiar with the PR. They might not understand all parts of it, but they should have at least a cursory understanding of the PR.

2. Have a meeting where the PR is explained in front of the group of reviewers. The reviewers will understand the PR better and they can ask questions in realtime.

3. Let folks review the PR after the meeting in case they spot anything else, or think of additional questions.

Most of the time PR review is done asynchronously, but doing most of the review in the meeting can also be a decent team building exercise.

Re: The Theatre of Pull Requests and Code Review

#299

Post by siva7 »

agentultra wrote: post #281
DSMan195276 wrote: post #277

Earlier quoted context omitted.

> I'd go read the original PR and the discussion that took place. Until your company switches code repos multiple times and all the PR history is gone or hard/impossible to track down. I will say, I don't usually make people clean up their commits and also usually recommend squashing PRs for any teams that aren't comfortable with `git`. When people do take the time to make a sensible commit history (when a PR warrant…

It also enables bisect to work properly. Bisecting on squashed commits is not usually helpful. You can still narrow down to the offending commit that introduced the error but… it’s somewhere in +1200/-325 lines of change. Good luck.

If your PR is + 1000 code lines long, you already made a mistake at the requirements and planning stage (like many teams do).

Re: The Theatre of Pull Requests and Code Review

#300

Post by singpolyma3 »

William_BB wrote: post #286
singpolyma3 wrote: post #269

Earlier quoted context omitted.

The PR UI on GitHub definitely leads to treating it as the unit of work. I consider this unfortunate for the most part, but the basic side effect is that I'll often end up submitting every commit as a separate PR so they actually get looked at

Respectfully, I disagree. I understand people have vastly different experiences and preferences. In my ideal world, a PR is a unit of work that has an in-state and an out-state. I don't have to see an initial commit within a PR with a full fledged spec and then wonder if any future commits within the same PR overrode those changes. A PR will rarely be clean from the start. The way it appears to me, if there's multipl…

Indeed, I agree. If a PR has multiple commits and those commits are atomic then by definition the PR is not atomic.
Post reply on HN