| Extremely valuable. It's the best way to transfer knowledge among engineers and control damage. It's more vital in bigger teams and teams that have something in production, used by a lot of people. A bad push breaks things on multiple levels, and the damage is multiplied by number of users. One person making two build breaking mistakes a year in a team of about 25 would break the build every week - CI/CD and auto review catches some of this, but PRs are good at catching the obvious. Plus regressions. I've been commenting that "this line prevents the logo printing bug for PNG in HN-1345". Often people don't know why the line is there and why it looks funky. But the new era hates comments. So we're reliant on humans who have faced bug HN-1345 to remember. In some cases, they've faced similar bugs in other jobs. I think most places overdo it though. 80-20 rule. Less than 20% of time should be spent on this. Most should be automated. I just check that it works. That it does what it should. That there's no gotchas ("triBeep() rings at 3 frequencies, not three times"). That it's not an architectural problem ("we don't put strings in the async because it leads to a memory leak and/or makes translations harder in the future") There's some quality control, but it often involves the above - don't name functions triBeep() and so on. Code style should be controlled by the linter. I never comment on whether it should be onChange vs onChanged, this is all bikeshedding and doesn't prevent bugs or increase productivity. |