For non-trivial reviews, when there are files with several changes, I tend to do the following in Git:
- Create local branch for the pr to review
- Squash if necessary to get everything in the one commit
- Soft reset, so all the changes are modifications in the working tree
- Go thru the modificiations “in situ” so to speak, so I get the entire context, with changes marked in the IDE, instead of just a few lines on either side.
Just curious if this is “a bit weird”, or something others do as well?
(ed: as others mentioned, a squash-merge and reset, or reset back without squashing, is the same, so step 2 isn’t necessary:))
Let’s step back. We are trying to talk about code review, right? That’s when you are getting a final check that your code looks roughly correct and does what you already planned for it to do with your colleagues, give or take a few details you couldn’t plan for. So at this point, usually at least one other person knows what you’re trying to do, is bought in, and should be able to review it.
I’ll concede that if you are desperate to merge a PR and people are on vacation or something, then you might want to grab someone else to review. I’d send them the PR and say, “let me know if you have questions.” Then if they do, you can have a quick chat or meet one on one.
That’s code review.
You are talking about some other thing. I’ve been in internal release meetings with lots of people that needed to understand the basics of a feature in order to support it or use it. That happens way after the PR is merged. And I’ve been in meetings to figure out how we should solve a problem, which happens before the PR is reviewed. I have gotten PSA emails or slack messages about cool new tools that make my life easier or changed our coding standards in the organization.
I have never been asked to meet with multiple people about what a PR is about before reviewing it.
IME those meetings only happen if the PR is objectionable upon review and it’s hard to negotiate what must be changed.
Only on very small projects.
As the team grows, having just 1 person review your code is simply not sufficient.
Even 2-3 may not be enough to sanity check if a larger new feature on a massive project is good. If it impacts the team and means a fundamental shift in methodology, then you need more voices weighing in.
Now, can you merge the PR in first, then have the meeting after? Sure, I guess, but why would you?
If the team reacts negatively to what you built, finds flaws in it, or simply just doesn’t get it because you overengineered, now you have to revert the PR and go back to the drawing board.
It makes tremendously more sense to bring folks impacted in on a swarmed live PR review as you go over what you did, where you did it, and why… before you merge it in.
At this point you can QnA, get suggestions, feedback, etc.
Now typically most of my response to live chat feedback is “aight, can you add that as a comment on the PR itself so I can see it after?” And then they go and do that asap, usually typing it up as I answer other folks questions. Because at this stage the PR is literally the perfect place for feedback to go.
I’ve been down the path of “1 person LGTMs the PR, huge new feature is added, I document it on the wiki rigorously, I post the new feature to chat… 1/10 people bother to use it and 9/10 give blank glassy stares when I bring it up”
It. Doesn’t. Work.
Period, lol. And don’t get me wrong, I wish it did, but people just don’t RTFM mate, that’s a fact of life a d the sooner you accept that, the easier the job gets.
K what do I know, it’s just what I’ve done on code bases with ~5 million lines of code, at the point where no one understands even half of it.
Everything about what you just wrote, and the way you represented it, signals you are precisely the type of individual that should take everything about what I wrote above very much to heart, friend.
If you have no idea what I’m talking about, I hope one day you do :)
You come off as incredibly patronizing. You should take that to heart.
There’s not really a nice way to frame “your post sounds like it was written by an extremely cringe teenager trying to cosplay as their idea of what constitutes a professional dev, demonstrated by the classic combination of ignoring everything prior written, attempting to represent a ball of mud as a badge of honor, and unironically trying to use lines of code count as a metric to measure by”
Literally checked off all the “lol sure bud” boxes in a single statement, and then if you aren’t picking up on the nuance, let me explain what I wrote after:
I hope you understand later how incredibly cringe what you wrote is, because the day you do is the day you have likely matured enough in knowledge and skill to call yourself a professional unironically, which is a good thing.
Until that day, stop prostrating shit like what you just wrote above if you ever want any developer worth their salt to take you even remotely seriously, otherwise you will likely find yourself the laughing stock very quickly of any serious circle.
Best of luck out there, and finally:
Next time someone is giving extremely useful advice, just write it down, don’t shit talk. That’s without a doubt the #1 divergence that separates the path between long term failure vs success.