This does seem like a potential issue if the PR is itself implementing more than one vertical slice of a feature. Then it could have been smaller and there might be wasted effort.
If the patches are small and well-organized then this isn’t necessarily a bad thing. It will take more than one day to review it, but it clearly took much more time to write it.
It’s harder to review. As a reviewer it’s difficult to know which code change is related to which task.
It’s harder to verify. Did you really test every change you made?
You might end up with a “hostage” situation. There might be a few code changes in the PR that looks good and is really wanted, but other code changes in the same PR of lower quality. As a reviewer, should you just let these lower quality code changes slide so you can bring in the code change you really want? Probably not, but you’re going to let it slide either way.
Right but it’s pretty rare that a tiny PR actually accomplishes a valuable user story.
So my point is just that lines of code is mostly irrelevant as long as it’s organized well and does no more than necessary to accomplish the agreed upon goal.
This does seem like a potential issue if the PR is itself implementing more than one vertical slice of a feature. Then it could have been smaller and there might be wasted effort.
If the patches are small and well-organized then this isn’t necessarily a bad thing. It will take more than one day to review it, but it clearly took much more time to write it.
True, but at the same time its very possible to go too small. A bunch of one line code reviews can really slow progress easily.
Stuffing multiple tasks into one PR is often bad.
Right but it’s pretty rare that a tiny PR actually accomplishes a valuable user story.
So my point is just that lines of code is mostly irrelevant as long as it’s organized well and does no more than necessary to accomplish the agreed upon goal.