The conversation

Lessons learnedExperience excerpt · English

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels …

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 necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result) - 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. - “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. …

Original discussionThe Theatre of Pull Requests and Code Review416 source comments · at import
8

Community replies 0

No community replies yet. Add your perspective.

Replies from the source 8

Direct replies available in this import. Original-language excerpts.

LE

My simple suggestion to my teams: PRs are emails to your team and to your future self. Framed in that context it's easier to carry the correct tone and think about scoping / what's important. --- > pedantically apply DRY to every situation I swear DRY has done more damage to the software industry from the developer side than it has done good because it has manifested into this big stick with which to bludgeon people without taking context into account.

Full original
MO

The "mid levels who consider themselves senior" are the exact type of people who I see saying what you're saying, i.e. * Yes, TDD on production code is nice in theory, but it doesnt work in my case . * Yes, short PRs are nice in theory, but it doesnt work in my case . In every case, as far as I can see, it meant "It does work, I just dont know how to do it". When I say "if you dont think it works in your case, come to me, Ill show you" they often demur and I end up with a huge PR anyway. In practice I dont think ive ever seen a long PR that wouldnt have benefitted from being strategically broken up, but every other day I see another one that should have been.

Full original
MO

Thank you, more people need to read this. The software industry seems packed with these strange gatekeeping structures that only hinder development. Focus on customer outcomes, and keep main clean.

Full original
SK

>nobody reads intermediate commit messages one by one on a PR, period. >Don’t waste your time writing stuff for no one. I've thought about that as I continue to write them. I think I can justify it by saying they are mostly for me. Can I describe what I'm trying to do with a specific push into a few items. It let's me reflect if I'm waiting too long between commits or if my ideas are getting too spread apart and really should be in two different branches that each have their own PRs. Then there is the rare case on a slower project where an item gets deprioritized and I come back to it weeks or even months later. Having the messages help me catch back up to speed. As such, I find the 20 seconds or so to type out 1 to 2 sentences to be worthwhile, even if the ones reviewing the eventual PR never check. I'm also not above throwing in a "ditto" or "fixed issue" when a single commit really is…

Full original
DA

> “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 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. You may be 100% correct in your particular case, but in general if senior devs are complaining that your code is sloppy and under-tested, maybe they aren't just being pedantic.

Full original
CH

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

Full original
JO

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

Full original
MD

With commit messages you miss the point. It’s more like the final test of the commit. If you can’t formulate easily what you did and why, then you need to rethink your changes.

Full original
Continue on Hacker News

Respect different perspectives. Share concrete experiences.

Sign in to post, reply, repost, and quote.