4 ms·

Agree with the overall sentiment but disagree with > A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreview

by epage 1y ago

Agree with the overall sentiment but disagree with > A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.

padjo 1y ago
It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.
SOLAR_FIELDS 1y ago
Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene. It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.
- StilesCrisis 1y ago
  You also need tools that support the workflow. I love small self-explanatory commits. In git, it's easy to do. Recently I switched to a Perforce organization and it's a disaster. P4 doesn't support stacked CLs and it dramatically hurts engineering quality. Everyone lands mega-CLs because it's the only supported workflow.
  - epage 1y ago
    Sorry to hear about the use of Perforce. Even the dinosaur of a company I used to work at shifted away. (granted, I know VCS like it are still good for assets)
  - skirmish 1y ago
    Have you tried using the git adapter to Perforce backend [1]? It should let you avoid mega-CLs. When Google used to use Perforce, an internal somewhat similar but a bit more advanced tool git5 was very popular. [1] https://git-scm.com/docs/git-p4 https://git-scm.com/docs/git-p4
    - StilesCrisis 1y ago
      The adapter breaks down on sufficiently massive repos. Unfortunately, there's a reason Perforce is still in use despite no one liking it. Git scales via sub-repos, but those have their own drawbacks, and no one is enthusiastic enough about this to champion it.
      - haradion 1y ago
        With the right workspace mapping, you can pull in just part of the depot, which makes the repo size a lot more manageable.
stillworks 1y ago
In some parts of the industry number of CRs and revisions-per-cr is tracked as a performance metric. Many people learn to game this to make their "numbers" appear good i.e. high number of CRs and low revisions per CR.
- osigurdson 1y ago
  Easy and fun to put metrics around random things. I'll start to squirm if you ask me to draw the connection between these metrics and NPV.
  - stillworks 1y ago
    The interesting phenomenon is the discovery of gamification of such metrics. I do see the value in breaking down larger chunks of work into logically smaller units of work and then produce multiple pull requests where needed. But some people are really clever and influential and manage to game these numbers into "apparent success".
    - osigurdson 1y ago
      It just becomes a huge cycle which is easier for everyone involved than doing actual work that benefits your customers.

by epage 1y ago

padjo 1y ago

It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.

SOLAR_FIELDS 1y ago

Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene. It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.

StilesCrisis 1y ago
You also need tools that support the workflow. I love small self-explanatory commits. In git, it's easy to do. Recently I switched to a Perforce organization and it's a disaster. P4 doesn't support stacked CLs and it dramatically hurts engineering quality. Everyone lands mega-CLs because it's the only supported workflow.
- epage 1y ago
  Sorry to hear about the use of Perforce. Even the dinosaur of a company I used to work at shifted away. (granted, I know VCS like it are still good for assets)
- skirmish 1y ago
  Have you tried using the git adapter to Perforce backend [1]? It should let you avoid mega-CLs. When Google used to use Perforce, an internal somewhat similar but a bit more advanced tool git5 was very popular. [1] https://git-scm.com/docs/git-p4 https://git-scm.com/docs/git-p4
  - StilesCrisis 1y ago
    The adapter breaks down on sufficiently massive repos. Unfortunately, there's a reason Perforce is still in use despite no one liking it. Git scales via sub-repos, but those have their own drawbacks, and no one is enthusiastic enough about this to champion it.
    - haradion 1y ago
      With the right workspace mapping, you can pull in just part of the depot, which makes the repo size a lot more manageable.

stillworks 1y ago

In some parts of the industry number of CRs and revisions-per-cr is tracked as a performance metric. Many people learn to game this to make their "numbers" appear good i.e. high number of CRs and low revisions per CR.

osigurdson 1y ago
Easy and fun to put metrics around random things. I'll start to squirm if you ask me to draw the connection between these metrics and NPV.
- stillworks 1y ago
  The interesting phenomenon is the discovery of gamification of such metrics. I do see the value in breaking down larger chunks of work into logically smaller units of work and then produce multiple pull requests where needed. But some people are really clever and influential and manage to game these numbers into "apparent success".
  - osigurdson 1y ago
    It just becomes a huge cycle which is easier for everyone involved than doing actual work that benefits your customers.