4 ms·

I'm one of the most obsessive team members when it comes to commit / PR size and hygiene. I want each PR to change just one thing, for the commit title to uniqu

by fl0ki 3y ago

I'm one of the most obsessive team members when it comes to commit / PR size and hygiene. I want each PR to change just one thing, for the commit title to uniquely identify it, and the rest of the commit description to explain everything a reviewer or future maintainer would want to know. But even I don't insist on any particular line count metrics. Sometimes doing just the one thing does mean a 1k line PR or more. That might be the MVP of a feature or refactor that's in a state that can be reviewed. Splitting it up into functions which can't yet be understood in their broader context makes them harder to review, not easier. I do try to split out any preliminary refactor which makes the big diff smaller and more focused, but sometimes that just shaves off 10 lines. It's good hygiene but it doesn't change the reviewer's experience much, and the commit description now has to pull the weight of explaining why this refactor came first. Stacking is definitely a huge pain in GitHub and after years of attempts I have mostly given up. I very occasionally include multiple commits in one PR, so reviewers can switch between focusing on one change and seeing it in broader context at their choosing. `git rebase --interactive` means amending the whole branch is still just one command with no third party tooling to buy or learn. I think it's a very reasonable compromise with the tools available today. The biggest limiation is they can still only approve the PR overall no matter how many commits it has, but when you do this you scope the PR appropriately for that. That's the Linux kernel model of splitting commits but still soliciting review for the lot of them.

raziel2p 3y ago
I'm starting to lean towards, this is a people problem and not a tools problem. As as senior engineer, either I want to review every single change that's being made and I should be doing pair programming anyway, and PR approval is almost purely symbolic. Or, I trust my colleague enough that I'll spend 5 minutes looking at their justification, explanation, testing strategy, rollout plan and decide to trust their code. Stacked PRs and insisting on small PRs seems to want to fill some gap in the middle between these two, which isn't necessarily the correct solution.
- fl0ki 3y ago
  I've worked in environments where we had several people per large project and reviewers could genuinely point out important context from parts of the project they were more familiar with. It was an awesome environment for learning not just the project but experienced engineering in general. Now I'm in an environment with several projects per person instead of several people per project. Now not only do we have a bus factor where nobody is familiar with anything but their own projects, even if they do review something they can't do much to anticipate issues or add context. Reviews have higher friction and less value. For my own projects, I have instead invested in really rich testing, far beyond any other project here. It not only lets me move forward without regressions, it means if someone else ever has to take over without me, the tests will teach them broader context and catch regressions. All crafted in a way that test expectations are still easy to update when changes are intentional. But I'm still the only one doing it this way. Most projects have 0 tests, or worse, lots of clunky tests of which 0 are useful. At best I'm hoping I can inspire others to follow even if it's only for their own sake at first. I'm not holding my breath for systemic change.
  - EchoChamberMan 3y ago
    Testing is for sure the way to go to keep maintenance burden low.
  - raziel2p 3y ago
    I think in situations like that, it's useful to take a step back and ask how much business value you're adding. If you really are the sole person working on a project, how likely is it that this one project having a bug or crash at some point, will super negatively impact the business? If the answer is "unlikely", or you're not sure, then do it for its own sake (you enjoy it) or do it for yourself (maybe it'll look good on your CV, or it's good practice) - but don't expect recognition for it.
    - fl0ki 3y ago
      I'm horrified to say that individual bugs we make can cost our large employer millions, yet leadership still overloads people with multiple projects on tight deadlines and loose engineering standards. I'm talking about global control systems in the critical path of user-facing traffic. But it's not as bad as it sounds; when I've seen these done by larger teams, they indulged scope creep and had no excuse not to. We get to say no a lot, and that's often better than having the resources to build more stuff that then compounds in complexity and has to be maintained forever. The way I've done things on specific projects is recognized a fair amount, but not yet replicated enough. I get trusted with the most mission-critical projects because I've proven I can do them with minimal risk. I've tried teaching the rest of the team how, but they still fall into bad habits again under pressure, and create a death spiral where their shortcuts taken under pressure create more pressure indefinitely. I make my deadlines because I invest in testability first and I'd always rather refactor earlier than later so tech debt doesn't compound. I don't mean anything dogmatic like TDD, I mean whatever gets me the most ROI on time and effort for a given kind of project. When time is the most constrained resource, it would be really silly to waste it on impedance mismatch between problems and practices.
munksbeer 3y ago
> Splitting it up into functions which can't yet be understood in their broader context makes them harder to review, not easier. Recently I wanted to add a hook to a certain part of our pipeline, which involved a lot of plumbing work (bad smell, but anyway). It was already about 200 lines of code and then some tests to ensure the hook was called. I raised that with a null implementation of the listener. I then raised a separate PR adding the actual listener I wanted, with busisness logic and tests for that logic. Am I understanding that some reviewers would object to the first PR because they can't understand the broader context? I think that's silly.
- fl0ki 3y ago
  Your null implementation did help put it into context to some extent. Even then, in general, the API surface sufficient for a null implementation may not be enough for some real implementations. What if at least one real implementation ended up needing another parameter, and making that parameter available required a cascade of changes through several other functions and types? If that's what happened, the second change would have to carry that weight, and the changes combined required more review overall than one change which had the final API from the start. I assume the way you wrote it is that you already had the real version ready so you knew the API was sufficient even when you only sent the null version for review. Your reviewer probably trusted you to do that, or at least assumed it was in your interests to do that. But if that wasn't assumed or asserted, the reviewer could rightly ask how you know the first version is the right design based only on the null version. I've definitely seen people come up with an architecture, API, or even network protocol based on what they hope will work without testing it on any real implementation yet. Just one detail from the real implementation can undermine the whole idea, scrapping any dev and review work done up to that point. Your change didn't have that problem, but if we're recommending principles for others then it's only fair to say this can be a problem. I don't ask for a review for any design I haven't already fleshed out to a working solution, and even then, what works for one real consumer may not generalize well enough to other real consumers. But I consider one the bare minimum, and I make sure reviewers know what that one real implementation is as context for review, even if it does happen to be in a separate change or even separate repo. At an extreme, it's like how you don't post an RFC that imagines a neat protocol that could exist, you post an RFC describing a protocol that has already been implemented and can now be generalized for broader interoperability. An internal API surface is the other extreme because it is much easier to evolve later, and a library API surface is somewhere in between, but all of them benefit from demonstrating real-world implementations of the abstract interface.

by fl0ki 3y ago

raziel2p 3y ago

I'm starting to lean towards, this is a people problem and not a tools problem. As as senior engineer, either I want to review every single change that's being made and I should be doing pair programming anyway, and PR approval is almost purely symbolic. Or, I trust my colleague enough that I'll spend 5 minutes looking at their justification, explanation, testing strategy, rollout plan and decide to trust their code. Stacked PRs and insisting on small PRs seems to want to fill some gap in the middle between these two, which isn't necessarily the correct solution.

fl0ki 3y ago
I've worked in environments where we had several people per large project and reviewers could genuinely point out important context from parts of the project they were more familiar with. It was an awesome environment for learning not just the project but experienced engineering in general. Now I'm in an environment with several projects per person instead of several people per project. Now not only do we have a bus factor where nobody is familiar with anything but their own projects, even if they do review something they can't do much to anticipate issues or add context. Reviews have higher friction and less value. For my own projects, I have instead invested in really rich testing, far beyond any other project here. It not only lets me move forward without regressions, it means if someone else ever has to take over without me, the tests will teach them broader context and catch regressions. All crafted in a way that test expectations are still easy to update when changes are intentional. But I'm still the only one doing it this way. Most projects have 0 tests, or worse, lots of clunky tests of which 0 are useful. At best I'm hoping I can inspire others to follow even if it's only for their own sake at first. I'm not holding my breath for systemic change.
- EchoChamberMan 3y ago
  Testing is for sure the way to go to keep maintenance burden low.
- raziel2p 3y ago
  I think in situations like that, it's useful to take a step back and ask how much business value you're adding. If you really are the sole person working on a project, how likely is it that this one project having a bug or crash at some point, will super negatively impact the business? If the answer is "unlikely", or you're not sure, then do it for its own sake (you enjoy it) or do it for yourself (maybe it'll look good on your CV, or it's good practice) - but don't expect recognition for it.
  - fl0ki 3y ago
    I'm horrified to say that individual bugs we make can cost our large employer millions, yet leadership still overloads people with multiple projects on tight deadlines and loose engineering standards. I'm talking about global control systems in the critical path of user-facing traffic. But it's not as bad as it sounds; when I've seen these done by larger teams, they indulged scope creep and had no excuse not to. We get to say no a lot, and that's often better than having the resources to build more stuff that then compounds in complexity and has to be maintained forever. The way I've done things on specific projects is recognized a fair amount, but not yet replicated enough. I get trusted with the most mission-critical projects because I've proven I can do them with minimal risk. I've tried teaching the rest of the team how, but they still fall into bad habits again under pressure, and create a death spiral where their shortcuts taken under pressure create more pressure indefinitely. I make my deadlines because I invest in testability first and I'd always rather refactor earlier than later so tech debt doesn't compound. I don't mean anything dogmatic like TDD, I mean whatever gets me the most ROI on time and effort for a given kind of project. When time is the most constrained resource, it would be really silly to waste it on impedance mismatch between problems and practices.

munksbeer 3y ago

> Splitting it up into functions which can't yet be understood in their broader context makes them harder to review, not easier. Recently I wanted to add a hook to a certain part of our pipeline, which involved a lot of plumbing work (bad smell, but anyway). It was already about 200 lines of code and then some tests to ensure the hook was called. I raised that with a null implementation of the listener. I then raised a separate PR adding the actual listener I wanted, with busisness logic and tests for that logic. Am I understanding that some reviewers would object to the first PR because they can't understand the broader context? I think that's silly.

fl0ki 3y ago
Your null implementation did help put it into context to some extent. Even then, in general, the API surface sufficient for a null implementation may not be enough for some real implementations. What if at least one real implementation ended up needing another parameter, and making that parameter available required a cascade of changes through several other functions and types? If that's what happened, the second change would have to carry that weight, and the changes combined required more review overall than one change which had the final API from the start. I assume the way you wrote it is that you already had the real version ready so you knew the API was sufficient even when you only sent the null version for review. Your reviewer probably trusted you to do that, or at least assumed it was in your interests to do that. But if that wasn't assumed or asserted, the reviewer could rightly ask how you know the first version is the right design based only on the null version. I've definitely seen people come up with an architecture, API, or even network protocol based on what they hope will work without testing it on any real implementation yet. Just one detail from the real implementation can undermine the whole idea, scrapping any dev and review work done up to that point. Your change didn't have that problem, but if we're recommending principles for others then it's only fair to say this can be a problem. I don't ask for a review for any design I haven't already fleshed out to a working solution, and even then, what works for one real consumer may not generalize well enough to other real consumers. But I consider one the bare minimum, and I make sure reviewers know what that one real implementation is as context for review, even if it does happen to be in a separate change or even separate repo. At an extreme, it's like how you don't post an RFC that imagines a neat protocol that could exist, you post an RFC describing a protocol that has already been implemented and can now be generalized for broader interoperability. An internal API surface is the other extreme because it is much easier to evolve later, and a library API surface is somewhere in between, but all of them benefit from demonstrating real-world implementations of the abstract interface.