top | item 45372506

(no title)

vinnymac | 5 months ago

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.

discuss

order

tcoff91|5 months ago

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.

nicksergeant|5 months ago

Yep. Self-reviewing your own PRs is a large boost to both yourself and the team, and often one of the first things I encourage new-ish developers to do.

- 90% of the time when you self-review your own PR, you're going to spot a bug or some incorrect assumption you made along the way. Or you'll see an opportunity to clean things up / make it better.

- Self-reviewing and annotating your reasons/thought process gives much more context to the actual reviewer, who likely only has a surface level understanding of what you're trying to do.

- It also signals to your team that you've taken the time to check your assumptions and verify you're solving the problem you say you are in the PR description.

goosejuice|5 months ago

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.

ljm|5 months ago

I do it quite often and it's great, because it helps contextualise some changes that might not seem to be intuitive.

You could argue this is what commits are for, but given how people use GitHub and PRs, it gives some extra visibility.

And if you're going to use AI to assist you when writing the code I would argue this self-review step is 100% mandatory.

vinnymac|5 months ago

Yep!

Adding context to both your commits and your code review tools pull requests / merge requests makes everyone's lives better. Including future you, who inevitably is looking at the PR or commit in the future due to an incident caused by said change.

I have been following this personal rule for well over a decade, and have never regretted it.

DerArzt|5 months ago

I've been doing this as part of my workflow for a few years now. My coworkers have expressed appreciation around that effort.

A nice side effect is that going through a self review and adding comments to the PR has helped me catch innumerable things that my coworkers never had to call me on.

jiveturkey|5 months ago

I often do this. It's a great way to highlight the areas you want a reviewer to focus on, the areas you are least happy about and want some collab. You can't always get collab pre-review, as you're writing. Plus you want to write it down and move on. Then you can async edit when the feedback comes. Not unlike a prose writing process.

Fishkins|5 months ago

Yeah, I never send a PR out without reviewing each commit myself and adding GitHub comments when I think it's relevant. Sometimes a PR is clear enough that I don't feel the need to add comments, though.