If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.
If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.
This will be accompanied by the sort of dev manager that thinks a KPI for "number of PRs merged" won't in any way be gamed or backfire.
I don't know what they're doing where you can do code reviews in 5-10 minutes, but in my decades doing this that only works for absolutely trivial changes.
The goal here is to make almost all CLs trivial enough that they can be reviewed quickly. You can compose almost any feature out of many small simple changes.
> You can compose almost any feature out of many small simple changes.
You can?
Yes, and you can waste a ton of a coworker's time doing it.
Say you're upgrading to a new library, it has a breaking change to an API. First, add `#if`s or similar around each existing change to the API that check for the existing library vs the new version, and error if the new version is found. No behavior change, one line of code, trivial PR. One PR per change. Bug your coworker for each.
Next, add calls to the new API, but don't actually change to use them (the `#if`s won't hit that condition). Another stack of trivial PRs. Your coworker now hates you.
Finally, swap the version over. Hopefully you tested this. Then make a final PR to do the actual upgrade.
For something less trivial than a single breaking upgrade, you can do the same shit. Conditionally compile so that your code doesn't actually get used in the version you do the PR in, you can split out to one PR per character changed! It'll be horrible, everyone will hate you, but you can split changes down to ridiculously small sizes.
Follow these guidelines and you'll be surprised how far you can go.
https://google.github.io/eng-practices/review/developer/smal...
So you want code added in 100 line sections behind one feature flag which we then suddenly turn on?
Isn't that exactly how Google's latest big cloud outage happened?
EDIT: referring to https://news.ycombinator.com/item?id=44274563
I should add that the idea complexity relates to LoC is also nonsense. Everyone that's been doing this for a while knows that what kills people are those one line errors which make assumptions about the values they are handling due to the surrounding code.
> only you hide the way all changes interact with each
Splitting the change does not prevent you from looking at diffs of any combination of those commits. (Including whole pr) You're not losing anything.
> at least 10x longer because an average approval time is often more than a day.
Why do you think it would take longer to review? Got any evidence?
I think approval time would take much longer. The issue is that while actual time spent in review may be shorter, there's a lot of context-switching time costs that increase with the number of PRs submitted.
No one on the team is just sitting there refreshing the list of PRs, ready to pick one up immediately. There's a delay between when the PR is marked as ready and when someone can actually get to it. Everyone is trying to get work done and have some time daily in flow state.
Imagine you have a change; you could do it as one PR that takes 1 hour to review, or 3 small PRs that each take 15 mins to review. The time spent in review may even be shorter for the small PRs, but if each PR has a delay of 1 hour before a reviewer can get to it, then the 3 PRs will take almost 4 hours before they're done, as opposed to just 2 hours for one big PR.
I don't think that's a realistic view of the timeline. I've done features as multiple PRs and there are really two cases:
1. I can't submit pieces until I have the final version. PRs go up at the same time and can be reviewed one after another immediately.
2. There's a very specific split that makes that feature two features in reality. Like adding a plugin system and the first plugin. Then the first part gets submitted while I still work on the second part and there's no delay on my side, because I'm still developing anyway.
Basically, I've never seen the "but if each PR has a delay of 1 hour before a reviewer can get to it," getting serialised in practice. It's still either one time or happening in the background.