8 ms·

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 themselve

by herval 1y ago

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. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing You want PRs because they help others absorb what you’re doing (they’ll have to read that same code sooner or later). You don’t want to create a performance theater.

leetrout 1y ago
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.
- BobbyJo 1y ago
  A great way to frame DRY that I heard from hackernews: "DRY things that are supposed to have the same behavior, not things that happen to have the same behavior"
  - collingreen 1y ago
    This is a really good way to put this. The "just because the do they same thing right now doesn't mean they _do the same thing_" concept is hard to convey!
  - tetha 1y ago
    I enjoy Sandi Metz' point there as well: Code just looking the same is not enough to call it duplication. Once you have to change two places looking the same to add a new feature or to fix a bug, then you have duplication and should centralize it.
    - michaelcampbell 1y ago
      I usually wait till 3; 2 is about the _very general_ point at which changing multiple places is about the same work as changing it to be centralized. 3 is almost always a better place to make that leap.
- m000 1y ago
  > PRs are emails to your team and to your future self. This should be commits though. Typically, developers would look for clues in this order: code -> code comment -> commit message -> PR text -> external document So commit messages puts the information closer to the user. One hop doesn't seem much, but the time saved adds up as you go. Also, as some other reader mentioned anecdotally, PRs may not be there forever. E.g. your team may migrate to a new platform PR text and reviews were left behind.
  - herval 1y ago
    In most sane development cycles I've seen (from 2-people teams to 100k people teams), intermediate commits disappear as soon as you merge your branch (in other words, you do short lived branches + squash merge). If you decide to do merges without squashing, then yes, you gotta have to have more hygiene on each individual commit. It creates a lot of unnecessary friction and it's guaranteed to be slower (devs can't use commits as checkpoints/savepoints on their work, but rather each commit becomes a fully fleshed out "intermediate final state"). The only situation where I see this making sense is if you share work on a branch with other engineers (which is also a bad idea).
    - m000 1y ago
      > devs can't use commits as checkpoints/savepoints on their work But they can! In git you can do whatever you want with your local/remote working branch. And after you're done it's pretty straightforward to massage it into a coherent series of commits (especially if you had been working with that in mind). > each commit becomes a fully fleshed out "intermediate final state" This is really a team decision. You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional.
      - herval 1y ago
        > And after you're done it's pretty straightforward to massage it into a coherent series of commits Why waste time? Just squash and merge, you have a single commit and it WORKS. Intermediate messages disappear and you have a single, atomic rollback point on your main branch > You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional. OR… squash and merge. Block merging with tests and compilation passing For anything in tech, there’s the frictionless way and the busywork way. Both of your examples are busywork that’s completely unnecessary if you just… squash and merge The best process is the process nobody needs to remember to do shit for it to work
- bear8642 1y ago
  > PRs are emails to your team and to your future self. Indeed! I've found many point on this discussion answered by the linux kernel idea of mailing lists where a change is discussed then approved, often with feedback acknowledged
MoreQARespect 1y ago
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.
- herval 1y ago
  Sure.
- BobbyJo 1y ago
  > Yes, TDD on production code is nice in theory, but it doesnt work in my case... Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided". I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.
  - MoreQARespect 1y ago
    Give me one example then. One is all it takes to disprove a rule. Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? The only exception i can think of is something where 99.9% of the changes are autogenerated (where i wouldnt really be reading it carefully anyway, so the length is immaterial...).
    - 1dom 1y ago
      I agreed with you initially. > I'm fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? To me, when I meet experts in any field, the quality that stands out isn't that they do everything to expert level, it's that they get everything done as they said they would. Sometimes that means big PRs, because that's the environment created, and the expert finds the way to get the job done. I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality, rather than just doing something because it's best practice and expecting everyone else to do the same. They ultimately get the thing done how they said they would.
mountainriver 1y ago
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.
SkyBelow 1y ago
>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 that small or insignificant. >“every commit must compile” I agree with your take this is overzealous, but to expand upon my previous point, if I know a commit on a branch won't compile (say just had something else come up and need to swap focus for a few days), then I'll try to make sure I call that out in my last message just in case anyone else happens to get put on the project. If I were to summarize my approach, treat PR messages seriously, but treat branch commit messages like sticky notes that will likely end up in the trash by week's end.
DavidWoof 1y ago
> “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.
- tossandthrow 1y ago
  > 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. This is a false dichotomy and an unproductive thing to focus at. Experienced engineers know when to make an abstraction and to not. It is based in the knowledge about project. Abstarct well and don't do compression. Easy said, and good engineers know how to do it.
- m4r71n 1y ago
  I actually find the relevant PR/MR discussion a lot more useful than the commit messages themselves. So any git blame is just to get a commit hash and look that up in GitLab/GitHub to see the entire change set and any comments around it. It makes me wish those comments were bundled with the merge commit somehow and could easily be accessed in the terminal where I'm viewing the git history.
  - sodapopcan 1y ago
    Not my experience. Often the single commit is all the context I need. If it's not, follow the merge to the ticket number to get more context.
- singpolyma3 1y ago
  Yes. I think many people have no culture of good commits, so they never use bisect or blame, so they never see the use of good commits. It's a cycle
chrislo 1y ago
> 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.
jonahx 1y ago
> 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.
- IshKebab 1y ago
  IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. A few exceptions: 1. When refactoring often your PR is "do an enormous search and replace, and then fix some stuff manually". In that case it's way easier to review if the mechanical stuff is in a separate commit. 2. Similarly when renaming and editing files, Git tracks it better if you do it in two commits. 3. Sometimes you genuinely have a big branch that's lasted months and has been worked on by many people and it's worth preserving history. Also I really really wish GitHub had proper support for stacked PRs.
  - mathstuf 1y ago
    This is truer now that `git bisect --first-parent` exists. But it didn't always. And even then, there are times you find out that there is "prep work" to land your feature. And a PR just to do some deck chair moving that makes a follow-up commit easier is kind of useless. I have done prep work as a separate PR, but this is usually when it is more extensive than the feature and it is worthwhile on its own. Another instance is a build system rewrite. There was a (short) story of the new system itself and then a commit per module on top of that. It landed as 300+ commits in a single PR. And it got rebased 2-3 times a week to try and keep up as more bits were migrated (and new infra added for things other bits needed). Partial landing would have been useless and "rewrite the build system" would have been utter hell for both me developing and anyone that tries to blame across it if it hadn't been split up at least that much. Basically, as with many things in software development, there are no black-and-white answers here.
mdavid626 1y ago
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.
sleepybrett 1y ago
> - 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 often do, In a larger PR or in one where it's hard to tell what is being accomplished, like this article articulates the commits can tell a story of the engineers journey to solution. Even if I review a commit that is largely undone by future commits that piece of history is often key to my understanding.
- bornfreddy 1y ago
  This. Just last week I have split a coworker's single-commit-MR into multiple commits so that I could distinguish between unrelated changes and to check smaller chunks of code. It worked beautifully.
joshlemer 1y ago
> nobody reads intermediate commit messages one by one on a PR I think it's fine to have a whole bunch of "WIP" commit messages on intermediate commits while the PR is in a draft stage, but then all of those garbage commits should really be squashed down into one commit and you should at least write a one liner that describes what the whole change is doing. I think it does materially make repo history harder to understand to merge in PR's with 10 garbage commits in them.
gwbas1c 1y ago
> “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing (With few exceptions,) I generally follow this practice; BUT, I think enforcing this on other developers feels like micromanagement. That being said, with few exceptions, committing code that doesn't compile feels like an incomplete sentence. (Sometimes on massive refactors I make commits that don't compile. It gives me a place to roll back to. If someone thinks this is poor practice, than I think they're putting principles in place of practicality.)
- herval 1y ago
  that's the whole point. A _branch_ is a unit of work that should be merged when done. As the owner of a branch, an engineer has the ability to move into intermediate states. The larger the codebase, the larger the possibility of something unexpected breaking or not compiling. Just like editing a large body of text - you will have "incomplete sentences" through the process. It's part of writing. Expecting others to write their drafts the same way you like is just silly - it's putting rigid principles ahead of anything else that matters.
epage 1y ago
While I agree about not having hard and fast rules, like LoC per PR, the principles of this are very relevant. When reviewing a conglomerate commit in a PR, I have to reverse engineer how the different changes interact to figure out the intent. I then have to do this on each update they make. Contrast that to when someone breaks up their commits where I can zoom through variable renames, extracting functions, etc to see the one line that change that all of that unblocked that makes the difference. Then if updates are pushed, I only have to worry about the commits that were updated. As for all commits compiling, that is helpful to review the individual commits. Both of these (small commits, all compiling) are also great for bisecting. You get pointed to a very small change that you can more easily analyze vs dealing with breakages or having to analyze a large change to find what the problem is.
criemen 1y ago
I agree with you that this shouldn't be 100% hard defaults, but it's a good standard to have, and imo it's valuable to be explain why one is deviating from it. > - 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) Oh but they sure can be reviewed more easily, because they are shorter? Doing so feels like less effort, and you get a dopamine hit from hitting that "submit review" button faster/more often (improved morale, and PR turnaround time!). Plus, if there's a longer discussion about X, it's great if it's not tangled up with Y and Z at the same time - allowing you to dig into X. > - nobody reads intermediate commit messages one by one on a PR, period. Come on, that's intellectually dishonest. 1. VSCode displays commit messages inline as blame for me (and many of my colleagues), so even when we don't read the commit messages one by one _on a PR_, I often read them later in the IDE (we don't squash merge PRs). I spend significantly more time reading code than writing, and commit messages, PR descriptions and linked issues provide extra context that is useful to me especially for complex code. If those messages were entirely unreadable, I'd be annoyed. 2. When someone invests time into telling a good story commit by commit, in my team they write "Review commit-by-commit is encouraged" in the PR description, to tell the reviewers that yes, they should read the individual commits, as that'll make understanding the PR easier. Often as reviewer, I follow that suggestion. > Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing It seams you're conflating "working on a feature" with "presenting it as PR to review". That's two very different things, and Edamagit in VSCode makes it so so easy to provide a reasonable commit history that hides some of your missteps, and to fill in commit messages.
- herval 1y ago
  > we don't squash merge PRs you need to be careful with every single commit message, every commit must compile, etc, in your case. My comments apply if you squash-merge, in which case all that commit-level care is not necessary since intermediate commits go away on merge. You’re probably making your life harder for no reason for avoiding squash-merge, but that’s just my opinion
muxator 1y ago
> - nobody reads intermediate commit messages one by one on a PR, period [...] > > - “every commit must compile” - again, unnecessary overzealousness. [...] In my part of the world both of these are true, and proudly so. We keep catching a myriad of errors, big and small. The history is easy to read, and helps anyone catching up with how a certain project evolved. I understand it might not be true for everyone, every team, in every line of business; but this sort of discipline pays off in quality oboth of the code _and_ the team members' abilities.
lamontcg 1y ago
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.
- mateo411 1y ago
  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.
  - lamontcg 1y ago
    Yeah, ideally the reviewers have been in standups with you so that it isn't all new as a concept to them to begin with, or there's generally been communication that you're going to land the plans for a nuclear reactor in their work queue. Hopefully you've been going around and around at a high level communicating back all the problems that you've hit and the design issues that emerged during exploratory surgery. Then, you definitely want to schedule at least one meeting to go over it. Which can become several meetings, including follow-up meetings with one or two individuals to pound out some specific issue. Depends on the complexity of the nuclear reactor.
jcalvinowens 1y ago
It's clear you've never worked on a large open source project... There are good reasons for all the practices you're thoughtlessly dismissing. I agree that for a common team of programmers working for a single company, the value isn't always there. But that's the easiest and least interesting case... in big distributed projects this stuff really matters.
1-more 1y ago
> 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 intermediate commits into ones that compile. Ignore this if you have real grown up CI where your PRs never stay open for more than a day.
- bob1029 1y ago
  > 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).
  - smcameron 1y ago
    > I've got an internal alarm that starts to go off somewhere around 72 hours. Nah, in my experience, if you've got good commit hygiene you can often merge even ancient commits. Here's a pretty hefty commit I merged five years after it was originally written, converting a ~100k line codebase from GTK to SDL2, written in 2015, committed in 2020, with tons of development in between, with "10 files changed, 777 insertions(+), 804 deletions(-)" https://github.com/smcameron/space-nerds-in-space/commit/4ab2495b04a13a9c282cde421c1646a74e306a95 https://github.com/smcameron/space-nerds-in-space/commit/4ab... I was expecting it to be a bit of a nightmare, but it really wasn't bad at all.
zzzeek 1y ago
I disagree with everything you wrote here. Prs must absolutely limit their scope both in terms of length as well as what they accomplish (e.g. don't do unrelated refactorings in a pr delivering something else), large features must absolutely be broken into individual commits if not individual PRs, I definitely read each one and each one definitely has to complete and pass tests 100%, else they are leaking details into the next or that would "fix" the problem. This is also absolutely nothing like "forcing TDD" in people and these are all practices that junior devs should absolutely be doing since it will help them to think about code, change and maintainability a ton.
keybored 1y ago
> - 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) This is why I gave up reading the article shortly after reaching the point about making a history with commit messages. The comments—even if it is on a Git forum—will just be full of people that either say that it’s a waste of time or that it is literally impossible for this to be practiced by anyone.[1] Your best bet is to find projects where this is practiced (and you don’t have to look far). But making the case to a general audience? No, too many loud voices that treat version control like “I am committing now because I need to pick up the dry-cleaning” arbitrary/random snapshot-maker. [1] No one, period? Sounds like a bit of a strict ontological rule to me.
smcameron 1y ago
> - “every commit must compile” - again, unnecessary overzealousness. So you're the one breaking git bisect all the time. Grrrr. Use stgit and make decent commits instead of rolling in the dirt like an animal.
osigurdson 1y ago
>> every commit must compile If every commit on the main branch must compile then why wouldn't it also compile in the PR branch? It doesn't make sense to ask people to review, then after that rebase and merge imo.
osigurdson 1y ago
>> You want PRs because they help others absorb what you’re doing That isn't really where it came from though. The idea was, if I want an open source maintainer to accept my changes, I make a request to pull them from my branch. Once the open source maintainer has merged it in, they own it. If they don't like it (even one little bit), they can reject it because quality / ownership / maintenance is completely on them. On a team environment where no one owns anything it is a little less clear what the value is. You want to incentivize the "betterness" of "something" and are using "broadened knowledge" as a proxy for that. Usually this just goes unexamined but really it would be good to establish how broad and deep you want this knowledge to be and work back from there - is the 5 minute PR review the best way to achieve it?
theonething 1y ago
> “every commit must compile” - again, unnecessary overzealousness. my understanding is that you commit when you are at the "good place", where the part of the code you are working on works. That way when you keep going and find yourself going in a direction that is not right, you can go back to the last good place. If your code doesn't even compile, that doesn't seem like a good place.
watwut 1y ago
> 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. Meh, most people wont address it or ask that dollar. It does not mean I did not read it, I chuckled and moved on. I do read every commit on PR chain and every line. I am not necessary super attentive reviewer or something, but I never accept it without at least formally looking at it.
DarkNova6 1y ago
Thank you so much. You speak from the bottom of my heart.
jiveturkey 1y ago
I write intermediate commit messages as notes to self. You don't always work continuously on the same PR. The commit messages are a useful context refresher. Why advocate against this anyway? If no one reads them, it harms no one. Just like personal blogs. However, the writing of the blog is the useful act, not the reading. Ironic that you are accusing TFA article of being an expert novice. I don't disagree your take on him / the article, but you are committing the same sin.
- herval 1y ago
  You missed the point entirely. The point is forcing others to do something that has no inherent value to them or to your process, just because you like it, is junior behavior.
  - jiveturkey 1y ago
    Who's forcing? I might have misread TFA I guess. My reading was that the guy attended a conference, enjoyed the storytelling kind of talk (I mean this is a tried and true approach, there are even flash card decks on the story technique), and wrote a blog to capture and crystallize what he liked about it as it applies to his daily activity of writing code. I didn't read anything there claiming it was the one true way and anything else is a bankrupt approach. If the point is about forcing someone to write commit essays, then yes I did miss it.
michaelcampbell 1y ago
> - “every commit must compile” - again, unnecessary overzealousness. Until you need to `git bisect`. Then you'll require that every commit compile, pass tests, etc.; even if that means rebase/squashing to do it.
- whatevaa 1y ago
  You don't bisect a merge/pull request. There is no need for it, unless it is a giant, but then your workflow is different. Main has clean history and every commit is good.
ornornor 1y ago
> reads intermediate commit messages > every commit must compile I’m in the opposite camp. Following these two practices often doesn’t make any difference but the few times it did saved me a ton of time. Dropping commits or rebasing is much easier when you have descriptive, atomic commits. It’s also helpful when performing git blame archeology to try and understand why this code looks so weird and has no context. It’s also useful when bisecting (not so much a problem with small PRs, quite handy as they grow bigger and bigger) As with everything it’s about context and circumstances. As you gain expérience you can appreciate and gauge when it’s required. When you don’t have the expérience then you follow rules so that you gain said expérience. That’s how I see it.

by herval 1y ago

leetrout 1y ago

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.

BobbyJo 1y ago
A great way to frame DRY that I heard from hackernews: "DRY things that are supposed to have the same behavior, not things that happen to have the same behavior"
- collingreen 1y ago
  This is a really good way to put this. The "just because the do they same thing right now doesn't mean they _do the same thing_" concept is hard to convey!
- tetha 1y ago
  I enjoy Sandi Metz' point there as well: Code just looking the same is not enough to call it duplication. Once you have to change two places looking the same to add a new feature or to fix a bug, then you have duplication and should centralize it.
  - michaelcampbell 1y ago
    I usually wait till 3; 2 is about the _very general_ point at which changing multiple places is about the same work as changing it to be centralized. 3 is almost always a better place to make that leap.
m000 1y ago
> PRs are emails to your team and to your future self. This should be commits though. Typically, developers would look for clues in this order: code -> code comment -> commit message -> PR text -> external document So commit messages puts the information closer to the user. One hop doesn't seem much, but the time saved adds up as you go. Also, as some other reader mentioned anecdotally, PRs may not be there forever. E.g. your team may migrate to a new platform PR text and reviews were left behind.
- herval 1y ago
  In most sane development cycles I've seen (from 2-people teams to 100k people teams), intermediate commits disappear as soon as you merge your branch (in other words, you do short lived branches + squash merge). If you decide to do merges without squashing, then yes, you gotta have to have more hygiene on each individual commit. It creates a lot of unnecessary friction and it's guaranteed to be slower (devs can't use commits as checkpoints/savepoints on their work, but rather each commit becomes a fully fleshed out "intermediate final state"). The only situation where I see this making sense is if you share work on a branch with other engineers (which is also a bad idea).
  - m000 1y ago
    > devs can't use commits as checkpoints/savepoints on their work But they can! In git you can do whatever you want with your local/remote working branch. And after you're done it's pretty straightforward to massage it into a coherent series of commits (especially if you had been working with that in mind). > each commit becomes a fully fleshed out "intermediate final state" This is really a team decision. You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional.
    - herval 1y ago
      > And after you're done it's pretty straightforward to massage it into a coherent series of commits Why waste time? Just squash and merge, you have a single commit and it WORKS. Intermediate messages disappear and you have a single, atomic rollback point on your main branch > You can allow intermediate commits to e.g. fail the tests, and add a tag to your main/master after each merge. Then you know that only the tagged commits are guaranteed to be fully functional. OR… squash and merge. Block merging with tests and compilation passing For anything in tech, there’s the frictionless way and the busywork way. Both of your examples are busywork that’s completely unnecessary if you just… squash and merge The best process is the process nobody needs to remember to do shit for it to work
bear8642 1y ago
> PRs are emails to your team and to your future self. Indeed! I've found many point on this discussion answered by the linux kernel idea of mailing lists where a change is discussed then approved, often with feedback acknowledged

MoreQARespect 1y ago

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.

herval 1y ago
Sure.
BobbyJo 1y ago
> Yes, TDD on production code is nice in theory, but it doesnt work in my case... Parent said something more along the lines of "they don't work in every case, and trying to force it in every case is misguided". I agree that too big is more common than too small with respect to PR size, but you aren't putting forward much of an argument against parents "there are no absolutes" argument by straw manning them.
- MoreQARespect 1y ago
  Give me one example then. One is all it takes to disprove a rule. Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? The only exception i can think of is something where 99.9% of the changes are autogenerated (where i wouldnt really be reading it carefully anyway, so the length is immaterial...).
  - 1dom 1y ago
    I agreed with you initially. > I'm fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? To me, when I meet experts in any field, the quality that stands out isn't that they do everything to expert level, it's that they get everything done as they said they would. Sometimes that means big PRs, because that's the environment created, and the expert finds the way to get the job done. I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality, rather than just doing something because it's best practice and expecting everyone else to do the same. They ultimately get the thing done how they said they would.

mountainriver 1y ago

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.

SkyBelow 1y ago

>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 that small or insignificant. >“every commit must compile” I agree with your take this is overzealous, but to expand upon my previous point, if I know a commit on a branch won't compile (say just had something else come up and need to swap focus for a few days), then I'll try to make sure I call that out in my last message just in case anyone else happens to get put on the project. If I were to summarize my approach, treat PR messages seriously, but treat branch commit messages like sticky notes that will likely end up in the trash by week's end.

DavidWoof 1y ago

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

tossandthrow 1y ago
> 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. This is a false dichotomy and an unproductive thing to focus at. Experienced engineers know when to make an abstraction and to not. It is based in the knowledge about project. Abstarct well and don't do compression. Easy said, and good engineers know how to do it.
m4r71n 1y ago
I actually find the relevant PR/MR discussion a lot more useful than the commit messages themselves. So any git blame is just to get a commit hash and look that up in GitLab/GitHub to see the entire change set and any comments around it. It makes me wish those comments were bundled with the merge commit somehow and could easily be accessed in the terminal where I'm viewing the git history.
- sodapopcan 1y ago
  Not my experience. Often the single commit is all the context I need. If it's not, follow the merge to the ticket number to get more context.
singpolyma3 1y ago
Yes. I think many people have no culture of good commits, so they never use bisect or blame, so they never see the use of good commits. It's a cycle

chrislo 1y ago

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

jonahx 1y ago

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

IshKebab 1y ago
IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. A few exceptions: 1. When refactoring often your PR is "do an enormous search and replace, and then fix some stuff manually". In that case it's way easier to review if the mechanical stuff is in a separate commit. 2. Similarly when renaming and editing files, Git tracks it better if you do it in two commits. 3. Sometimes you genuinely have a big branch that's lasted months and has been worked on by many people and it's worth preserving history. Also I really really wish GitHub had proper support for stacked PRs.
- mathstuf 1y ago
  This is truer now that `git bisect --first-parent` exists. But it didn't always. And even then, there are times you find out that there is "prep work" to land your feature. And a PR just to do some deck chair moving that makes a follow-up commit easier is kind of useless. I have done prep work as a separate PR, but this is usually when it is more extensive than the feature and it is worthwhile on its own. Another instance is a build system rewrite. There was a (short) story of the new system itself and then a commit per module on top of that. It landed as 300+ commits in a single PR. And it got rebased 2-3 times a week to try and keep up as more bits were migrated (and new infra added for things other bits needed). Partial landing would have been useless and "rewrite the build system" would have been utter hell for both me developing and anyone that tries to blame across it if it hadn't been split up at least that much. Basically, as with many things in software development, there are no black-and-white answers here.

mdavid626 1y ago

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.

sleepybrett 1y ago

> - 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 often do, In a larger PR or in one where it's hard to tell what is being accomplished, like this article articulates the commits can tell a story of the engineers journey to solution. Even if I review a commit that is largely undone by future commits that piece of history is often key to my understanding.

bornfreddy 1y ago
This. Just last week I have split a coworker's single-commit-MR into multiple commits so that I could distinguish between unrelated changes and to check smaller chunks of code. It worked beautifully.

joshlemer 1y ago

> nobody reads intermediate commit messages one by one on a PR I think it's fine to have a whole bunch of "WIP" commit messages on intermediate commits while the PR is in a draft stage, but then all of those garbage commits should really be squashed down into one commit and you should at least write a one liner that describes what the whole change is doing. I think it does materially make repo history harder to understand to merge in PR's with 10 garbage commits in them.

gwbas1c 1y ago

> “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing (With few exceptions,) I generally follow this practice; BUT, I think enforcing this on other developers feels like micromanagement. That being said, with few exceptions, committing code that doesn't compile feels like an incomplete sentence. (Sometimes on massive refactors I make commits that don't compile. It gives me a place to roll back to. If someone thinks this is poor practice, than I think they're putting principles in place of practicality.)

herval 1y ago
that's the whole point. A _branch_ is a unit of work that should be merged when done. As the owner of a branch, an engineer has the ability to move into intermediate states. The larger the codebase, the larger the possibility of something unexpected breaking or not compiling. Just like editing a large body of text - you will have "incomplete sentences" through the process. It's part of writing. Expecting others to write their drafts the same way you like is just silly - it's putting rigid principles ahead of anything else that matters.

epage 1y ago

While I agree about not having hard and fast rules, like LoC per PR, the principles of this are very relevant. When reviewing a conglomerate commit in a PR, I have to reverse engineer how the different changes interact to figure out the intent. I then have to do this on each update they make. Contrast that to when someone breaks up their commits where I can zoom through variable renames, extracting functions, etc to see the one line that change that all of that unblocked that makes the difference. Then if updates are pushed, I only have to worry about the commits that were updated. As for all commits compiling, that is helpful to review the individual commits. Both of these (small commits, all compiling) are also great for bisecting. You get pointed to a very small change that you can more easily analyze vs dealing with breakages or having to analyze a large change to find what the problem is.

criemen 1y ago

I agree with you that this shouldn't be 100% hard defaults, but it's a good standard to have, and imo it's valuable to be explain why one is deviating from it. > - 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) Oh but they sure can be reviewed more easily, because they are shorter? Doing so feels like less effort, and you get a dopamine hit from hitting that "submit review" button faster/more often (improved morale, and PR turnaround time!). Plus, if there's a longer discussion about X, it's great if it's not tangled up with Y and Z at the same time - allowing you to dig into X. > - nobody reads intermediate commit messages one by one on a PR, period. Come on, that's intellectually dishonest. 1. VSCode displays commit messages inline as blame for me (and many of my colleagues), so even when we don't read the commit messages one by one _on a PR_, I often read them later in the IDE (we don't squash merge PRs). I spend significantly more time reading code than writing, and commit messages, PR descriptions and linked issues provide extra context that is useful to me especially for complex code. If those messages were entirely unreadable, I'd be annoyed. 2. When someone invests time into telling a good story commit by commit, in my team they write "Review commit-by-commit is encouraged" in the PR description, to tell the reviewers that yes, they should read the individual commits, as that'll make understanding the PR easier. Often as reviewer, I follow that suggestion. > Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing It seams you're conflating "working on a feature" with "presenting it as PR to review". That's two very different things, and Edamagit in VSCode makes it so so easy to provide a reasonable commit history that hides some of your missteps, and to fill in commit messages.

herval 1y ago
> we don't squash merge PRs you need to be careful with every single commit message, every commit must compile, etc, in your case. My comments apply if you squash-merge, in which case all that commit-level care is not necessary since intermediate commits go away on merge. You’re probably making your life harder for no reason for avoiding squash-merge, but that’s just my opinion

muxator 1y ago

> - nobody reads intermediate commit messages one by one on a PR, period [...] > > - “every commit must compile” - again, unnecessary overzealousness. [...] In my part of the world both of these are true, and proudly so. We keep catching a myriad of errors, big and small. The history is easy to read, and helps anyone catching up with how a certain project evolved. I understand it might not be true for everyone, every team, in every line of business; but this sort of discipline pays off in quality oboth of the code _and_ the team members' abilities.

lamontcg 1y ago

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.

mateo411 1y ago
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.
- lamontcg 1y ago
  Yeah, ideally the reviewers have been in standups with you so that it isn't all new as a concept to them to begin with, or there's generally been communication that you're going to land the plans for a nuclear reactor in their work queue. Hopefully you've been going around and around at a high level communicating back all the problems that you've hit and the design issues that emerged during exploratory surgery. Then, you definitely want to schedule at least one meeting to go over it. Which can become several meetings, including follow-up meetings with one or two individuals to pound out some specific issue. Depends on the complexity of the nuclear reactor.

jcalvinowens 1y ago

It's clear you've never worked on a large open source project... There are good reasons for all the practices you're thoughtlessly dismissing. I agree that for a common team of programmers working for a single company, the value isn't always there. But that's the easiest and least interesting case... in big distributed projects this stuff really matters.

1-more 1y ago

> 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 intermediate commits into ones that compile. Ignore this if you have real grown up CI where your PRs never stay open for more than a day.

bob1029 1y ago
> 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).
- smcameron 1y ago
  > I've got an internal alarm that starts to go off somewhere around 72 hours. Nah, in my experience, if you've got good commit hygiene you can often merge even ancient commits. Here's a pretty hefty commit I merged five years after it was originally written, converting a ~100k line codebase from GTK to SDL2, written in 2015, committed in 2020, with tons of development in between, with "10 files changed, 777 insertions(+), 804 deletions(-)" https://github.com/smcameron/space-nerds-in-space/commit/4ab2495b04a13a9c282cde421c1646a74e306a95 https://github.com/smcameron/space-nerds-in-space/commit/4ab... I was expecting it to be a bit of a nightmare, but it really wasn't bad at all.

zzzeek 1y ago

I disagree with everything you wrote here. Prs must absolutely limit their scope both in terms of length as well as what they accomplish (e.g. don't do unrelated refactorings in a pr delivering something else), large features must absolutely be broken into individual commits if not individual PRs, I definitely read each one and each one definitely has to complete and pass tests 100%, else they are leaking details into the next or that would "fix" the problem. This is also absolutely nothing like "forcing TDD" in people and these are all practices that junior devs should absolutely be doing since it will help them to think about code, change and maintainability a ton.

keybored 1y ago

> - 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) This is why I gave up reading the article shortly after reaching the point about making a history with commit messages. The comments—even if it is on a Git forum—will just be full of people that either say that it’s a waste of time or that it is literally impossible for this to be practiced by anyone.[1] Your best bet is to find projects where this is practiced (and you don’t have to look far). But making the case to a general audience? No, too many loud voices that treat version control like “I am committing now because I need to pick up the dry-cleaning” arbitrary/random snapshot-maker. [1] No one, period? Sounds like a bit of a strict ontological rule to me.

smcameron 1y ago

> - “every commit must compile” - again, unnecessary overzealousness. So you're the one breaking git bisect all the time. Grrrr. Use stgit and make decent commits instead of rolling in the dirt like an animal.

osigurdson 1y ago

>> every commit must compile If every commit on the main branch must compile then why wouldn't it also compile in the PR branch? It doesn't make sense to ask people to review, then after that rebase and merge imo.

osigurdson 1y ago

>> You want PRs because they help others absorb what you’re doing That isn't really where it came from though. The idea was, if I want an open source maintainer to accept my changes, I make a request to pull them from my branch. Once the open source maintainer has merged it in, they own it. If they don't like it (even one little bit), they can reject it because quality / ownership / maintenance is completely on them. On a team environment where no one owns anything it is a little less clear what the value is. You want to incentivize the "betterness" of "something" and are using "broadened knowledge" as a proxy for that. Usually this just goes unexamined but really it would be good to establish how broad and deep you want this knowledge to be and work back from there - is the 5 minute PR review the best way to achieve it?

theonething 1y ago

> “every commit must compile” - again, unnecessary overzealousness. my understanding is that you commit when you are at the "good place", where the part of the code you are working on works. That way when you keep going and find yourself going in a direction that is not right, you can go back to the last good place. If your code doesn't even compile, that doesn't seem like a good place.

watwut 1y ago

> 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. Meh, most people wont address it or ask that dollar. It does not mean I did not read it, I chuckled and moved on. I do read every commit on PR chain and every line. I am not necessary super attentive reviewer or something, but I never accept it without at least formally looking at it.

DarkNova6 1y ago

Thank you so much. You speak from the bottom of my heart.

jiveturkey 1y ago

I write intermediate commit messages as notes to self. You don't always work continuously on the same PR. The commit messages are a useful context refresher. Why advocate against this anyway? If no one reads them, it harms no one. Just like personal blogs. However, the writing of the blog is the useful act, not the reading. Ironic that you are accusing TFA article of being an expert novice. I don't disagree your take on him / the article, but you are committing the same sin.

herval 1y ago
You missed the point entirely. The point is forcing others to do something that has no inherent value to them or to your process, just because you like it, is junior behavior.
- jiveturkey 1y ago
  Who's forcing? I might have misread TFA I guess. My reading was that the guy attended a conference, enjoyed the storytelling kind of talk (I mean this is a tried and true approach, there are even flash card decks on the story technique), and wrote a blog to capture and crystallize what he liked about it as it applies to his daily activity of writing code. I didn't read anything there claiming it was the one true way and anything else is a bankrupt approach. If the point is about forcing someone to write commit essays, then yes I did miss it.

michaelcampbell 1y ago

> - “every commit must compile” - again, unnecessary overzealousness. Until you need to `git bisect`. Then you'll require that every commit compile, pass tests, etc.; even if that means rebase/squashing to do it.

whatevaa 1y ago
You don't bisect a merge/pull request. There is no need for it, unless it is a giant, but then your workflow is different. Main has clean history and every commit is good.

ornornor 1y ago

> reads intermediate commit messages > every commit must compile I’m in the opposite camp. Following these two practices often doesn’t make any difference but the few times it did saved me a ton of time. Dropping commits or rebasing is much easier when you have descriptive, atomic commits. It’s also helpful when performing git blame archeology to try and understand why this code looks so weird and has no context. It’s also useful when bisecting (not so much a problem with small PRs, quite handy as they grow bigger and bigger) As with everything it’s about context and circumstances. As you gain expérience you can appreciate and gauge when it’s required. When you don’t have the expérience then you follow rules so that you gain said expérience. That’s how I see it.