3 ms·

I believe that there's certain things (particularly ambitious net new feature development) that isn't served well by artificially minimizing PR size. You shoul

by sb8244 3y ago

I believe that there's certain things (particularly ambitious net new feature development) that isn't served well by artificially minimizing PR size. You should absolutely strive to keep smaller PRs, but I've frequently seen this become "you should only have small PRs."

0xbadcafebee 3y ago
Honestly "you should only have small PRs" is one of those cultural things more people should invest in. Small changes really are both faster and less bug-prone. To make it work, you have to think differently and code differently. You have to think in advance: "does what I want to do require changing a lot?" If so, think about how else you can solve your problem. Or when implementing something for the first time, think about how difficult it would be for someone to change it. You end up building things with high cohesion, low coupling, object factories, etc. It makes for very different code that is more maintainable, flexible, easier to change. You end up not needing a big PR, or the changes are to a single high-cohesion component so it's much easier to review than changing 10 different components.
jacobegold 3y ago
Net new feature development is often 20% or less of the job. With most guidelines like this, it's always possible to find exceptions -- but understanding why you're making the tradeoff in that situation is key. With that said, using methods like stacking and feature flagging, I've been finding even new feature development to be possible while keeping my PRs to roughly 5 files changed or less.
k_dumez 3y ago
> artificially minimizing PR size Not sure I understand the "artificial" part here. There's nothing "artificial" about breaking up your larger changes into smaller PRs. It's just good practice. Helps reviewers who are reviewing the code, and helps the author be more focused with their changes. Even in net new feature development it's a good idea to break up your large changes to something more manageable. Sorry if I'm not understanding, what do you believe the downside to be?
- sb8244 3y ago
  It's artificial to break up a PR to satisfy the rule of small PRs. Often you need the full context when evaluating a new feature end to end. Or you spend 2 days splitting up a PR into smaller PRs so that a person can review it in 30 minutes instead of 2 hours. I can't say I've ever seen benefit from it both as a reviewer or as a developer, but it could be an effect of different companies and different teams.
  - CuriousCosmic 3y ago
    I'm not sure I agree? If your code isn't trivial to compartmentalize changes then that might be a code smell. I'd agree keeping the unified context is preferable but it's probably easier to do that by having developers rebase their changes into discrete commits that can be reviewed one by one.
    - sb8244 3y ago
      It's not necessarily about the units of code, it's about the bigger picture. Let's give a hypothetical situation where you split up a PR by the backend and frontend components, but you have some extra fields and endpoints that are not used in your final product. They accidentally get left in because you miss them. I believe the chance that it would be caught in review is significantly higher in a unified PR review instead of artificially splitting front and back into separate PRs. And if you need to have the 2 PRs open side by side, then why split it up in the first place?
      - CuriousCosmic 3y ago
        > And if you need to have the 2 PRs open side by side, then why split it up in the first place? I agree however I think PR's should be split up. Just not into separate PRs. The solution is to actually start reviewing PRs by commit (you can do this in the PR web interface). Everything is split up and can be reviewed separately but you can still see the final diff and while each discrete unit is reviewable, the greater feature is also still one discrete reviewable item.

by sb8244 3y ago

0xbadcafebee 3y ago

Honestly "you should only have small PRs" is one of those cultural things more people should invest in. Small changes really are both faster and less bug-prone. To make it work, you have to think differently and code differently. You have to think in advance: "does what I want to do require changing a lot?" If so, think about how else you can solve your problem. Or when implementing something for the first time, think about how difficult it would be for someone to change it. You end up building things with high cohesion, low coupling, object factories, etc. It makes for very different code that is more maintainable, flexible, easier to change. You end up not needing a big PR, or the changes are to a single high-cohesion component so it's much easier to review than changing 10 different components.

jacobegold 3y ago

Net new feature development is often 20% or less of the job. With most guidelines like this, it's always possible to find exceptions -- but understanding why you're making the tradeoff in that situation is key. With that said, using methods like stacking and feature flagging, I've been finding even new feature development to be possible while keeping my PRs to roughly 5 files changed or less.

k_dumez 3y ago

> artificially minimizing PR size Not sure I understand the "artificial" part here. There's nothing "artificial" about breaking up your larger changes into smaller PRs. It's just good practice. Helps reviewers who are reviewing the code, and helps the author be more focused with their changes. Even in net new feature development it's a good idea to break up your large changes to something more manageable. Sorry if I'm not understanding, what do you believe the downside to be?

sb8244 3y ago
It's artificial to break up a PR to satisfy the rule of small PRs. Often you need the full context when evaluating a new feature end to end. Or you spend 2 days splitting up a PR into smaller PRs so that a person can review it in 30 minutes instead of 2 hours. I can't say I've ever seen benefit from it both as a reviewer or as a developer, but it could be an effect of different companies and different teams.
- CuriousCosmic 3y ago
  I'm not sure I agree? If your code isn't trivial to compartmentalize changes then that might be a code smell. I'd agree keeping the unified context is preferable but it's probably easier to do that by having developers rebase their changes into discrete commits that can be reviewed one by one.
  - sb8244 3y ago
    It's not necessarily about the units of code, it's about the bigger picture. Let's give a hypothetical situation where you split up a PR by the backend and frontend components, but you have some extra fields and endpoints that are not used in your final product. They accidentally get left in because you miss them. I believe the chance that it would be caught in review is significantly higher in a unified PR review instead of artificially splitting front and back into separate PRs. And if you need to have the 2 PRs open side by side, then why split it up in the first place?
    - CuriousCosmic 3y ago
      > And if you need to have the 2 PRs open side by side, then why split it up in the first place? I agree however I think PR's should be split up. Just not into separate PRs. The solution is to actually start reviewing PRs by commit (you can do this in the PR web interface). Everything is split up and can be reviewed separately but you can still see the final diff and while each discrete unit is reviewable, the greater feature is also still one discrete reviewable item.