Hacker News Reader

I twitched when the author's FIRST point was about code style; in this day and age that should not be up for debate anymore, run it through a formatter, change the formatter's rules if you don't like it, and have your CI reject it if the user did not run the formatter before anyone has a look at it. Code style is a distraction that a reviewer should not have to spend their focus on, not when the REAL problem is yet to be reviewed.

Yeah, I am totally with you on this one. Code style shouldn't be up for debate anymore. Nowadays we have plenty of linters and code style fixers that automates the whole code style of the project. That wasn't even the point I was trying to share on this article. PR and PP are on another level, focus on sharing knowledge rather than "code style", for sure :)

> If "usually" PRs result in code style or architecture discussions, I'd suggest you have a team problem, not a process one. Start with the humans.

+1

> Not mentioned is one of the other big benefits of PRs (which applies no matter if you pair or are a solo project): it enforces some good discipline.

I found that pairing and frequent rotations result in a much more uniform adoption of good coding practices, following working agreements, etc... It just happens much more frictionlessly since people learn faster by doing. Again, frequent rotations and ensuring that people mix as much as possible is key. (one company I worked for adopted a so called "promiscuity table" to keep track of that)

Also, to quote someone more experienced than me "I've never seen a +300LOC PR that didn't look good!"

Counterpoint: I've reviewed multi-thousand line PRs (with dozens of commits) ... those took me days of 8 hour review, with multiple issues found. (To be fair, when I first skimmed it, it looked great to me, but a deeper dive found lots of things that could be improved.) These kinds of large PRs was how the gentleman I worked with did pairing and it worked out pretty well (surprisingly, don't try this at home).

I suspect this is the exception that proves the rule, not really an alternative rule.

I added linters and prettier. We don't have this discussion.

Anything that isn't important I prefix with nitpick and my team knows to ignore if they want to.

Actual feedback should be based on functionality.

Having said that, sometimes pair programming helps. It sucks when you're remote. And it really sucks when you have tiny tiny desks as is the new startup trend.

I honestly miss nice big cubicles. Say what you will, they served a great purpose. Every eng I knew with a nice big cubicle had a 2nd keyboard for the other person that sat in.

>>> If "usually" PRs result in code style or architecture discussions, I'd suggest you have a team problem, not a process one. Start with the humans.

True, Why is code being written without the architecture and initial design hashed out?

Do an initial high level design, discuss with your team, hash out any issues, then start with the implementation.

The PR code review should be a review of the implementation and not the design.