Hacker News Reader

Comment by DSMan195276 | Hacker News Reader

DSMan195276

> I'd go read the original PR and the discussion that took place.

Until your company switches code repos multiple times and all the PR history is gone or hard/impossible to track down.

I will say, I don't usually make people clean up their commits and also usually recommend squashing PRs for any teams that aren't comfortable with `git`. When people do take the time to make a sensible commit history (when a PR warrants more than one commit) it makes looking back through their code history to understand what was going on 1000% easier. It also forces people to actually look over all of their changes, which is something I find a lot of people don't bother to do and their code quality suffers a lot as a result.

View on HN

It also enables bisect to work properly.

Bisecting on squashed commits is not usually helpful. You can still narrow down to the offending commit that introduced the error but… it’s somewhere in +1200/-325 lines of change. Good luck.

If your PR is + 1000 code lines long, you already made a mistake at the requirements and planning stage (like many teams do).

I don't agree, for example my team includes yarn.lock in the commit which adds quite a few lines to the PR.

This sounds unattainable to me. For code bases in the 2 million or more lines range, something as simple as refactoring the name of a poorly named base class can hit 5000 lines. It also was not a mistake with the original name it had, but you'd still like to change it to make it more readable given the evolution of the codebase. You would not split that up into multiple commits because that would be a mess and it would not compile unless done in one commit.

How is this a mistake?

Such PR's shouldn't be the norm but the exception. What happens way more often is that such refactoring happen in addition to other criteria on the same PR. In high-functioning teams i've worked this is usually done as a separate PR / change, as they are aware of the complexity this operation adds by mixing it with scope-related changes and that refactoring shouldn't be in the scope of the original change.

Or finding what bug was reintroduced in a +13/-14398

Reply on news.ycombinator.com