3 ms·

> When refactoring and preventing huge-ass PRs: “If I’d have changed all the tests first then I would have seen I had 52 files to change and that was obviously

> When refactoring and preventing huge-ass PRs: “If I’d have changed all the tests first then I would have seen I had 52 files to change and that was obviously gonna be too big but I was messing with the code first and not the tests.” Is breaking it up worth it? My 2 cents: There are two things to consider: 1. reviewability 2. deployment risk If it takes a colleague 3 days to review your code, your PR is too big. If you panic, on the thought of deploying this ever, your PR is too big. On the other side: - Huge PRs that only change formatting are fine. Easy review. Low risk if properly automated/tested. - Largs PRs that are feature neutral are acceptable as long as they are reviewable. - PRs that refactor 2000LOC, fix 2 bugs and add 3 features are not a good idea.

einpoklum 7y ago
The thing is: 1. Some people balk at you making lots of small PRs, because it feels like noise to them. 2. Some people/organizations make you do a lot of work on a separate branch and then want you to PR just once for the final feature you're implementing.
- lonelappde 7y ago
  1. Commits can be merged/squashed when merged upstream. 2. That's fine, your branch has the series of small PRs that people can review.
foobarian 7y ago
There are certain ways to organize large PRs to make them easier to review and deploy. Some examples: - if a large refactor moves code around in addition to changing logic, omit the pure moves from the diff and provide a list of files moved in a description - if a large refactor involves a lot of renaming, or other mechanical manipulation, in addition to a little bit of logic changes, split those apart. That way there will be one large mechanical diff, where you can state in the description, "Yes I changed 500 files and there are 20 pages of diffs but the nature of changes is the same as in the first file." This is vastly easier to review for your peers. - A useful safe deployment trick, when feasible: guard new functionality with a kill switch, as well as some kind of sampling gate (e.g. use new logic only where customer id % 31 == 0). Then when proved remove old code in a followup.

einpoklum 7y ago

The thing is: 1. Some people balk at you making lots of small PRs, because it feels like noise to them. 2. Some people/organizations make you do a lot of work on a separate branch and then want you to PR just once for the final feature you're implementing.

lonelappde 7y ago
1. Commits can be merged/squashed when merged upstream. 2. That's fine, your branch has the series of small PRs that people can review.

foobarian 7y ago

There are certain ways to organize large PRs to make them easier to review and deploy. Some examples: - if a large refactor moves code around in addition to changing logic, omit the pure moves from the diff and provide a list of files moved in a description - if a large refactor involves a lot of renaming, or other mechanical manipulation, in addition to a little bit of logic changes, split those apart. That way there will be one large mechanical diff, where you can state in the description, "Yes I changed 500 files and there are 20 pages of diffs but the nature of changes is the same as in the first file." This is vastly easier to review for your peers. - A useful safe deployment trick, when feasible: guard new functionality with a kill switch, as well as some kind of sampling gate (e.g. use new logic only where customer id % 31 == 0). Then when proved remove old code in a followup.