3 ms·
Also there is something to be said that code reviews also work well with code that is meant to be reviewed. The worst kind of peer review happens on PRs that a
by bobthepanda 2y ago
Also there is something to be said that code reviews also work well with code that is meant to be reviewed.
The worst kind of peer review happens on PRs that are thousands of lines because nobody wants to read all that and things will be missed. Where I have seen successful code review is where people break code into reviewable bits, and those individual reviews are so fast that they actually end up bring completed faster than if it had been one giant PR.
- anonymoushn 2y agoHow much additional time is needed to break a self-contained change that's the smallest it can reasonably be without breaking anything into a bunch of smaller changes though?
- withinboredom 2y agoLike 10-15 minutes .... git co master git co my-branch -- . git add -up . # select changes relevant to first pr git commit git reset HEAD --hard # and again...
- anonymoushn 2y agoThe question was specifically about scenarios in which this approach wouldn't work, for example because your team doesn't want to approve PRs containing only dead code or because any subset of the change won't compile or won't preserve correct behavior without the others pieces.
- dkdbejwi383 2y agoIt helps to have the right tooling in place to ship "incomplete" work, e.g. feature flags so that you can ship a very light and not ready for end-users version of some feature, and continue to iterate on it in smaller PRs. e.g. first pass adds a new screen and just dumps the output second pass adds input fields and controls next pass adds validation then add animations etc
- mewpmewp2 2y agoIt sounds so good in theory, but in practice: 1. Frequently old code needs to be touched or refactored. Feature flag would not be enough. 2. Even feature flag itself can be a risky addition, and might affect existing customer usage. Most of the time old code does need to be touched, there really aren't those perfect new isolated features, at least in my experience.
- bobthepanda 2y agoIf anything refactors should be behind feature flags *because* they are so disruptive.
- jappgar 2y agoIMO this is a terrible approach, and why I hate the way feature-flags are used nowadays. For example, I'm not approving anything without input validation (frontend or backend). I have no idea if you're actually going to add validation later before the fflag is removed. "Trust me bro" doesn't work for me.
- bobthepanda 2y agoI mean you can have validation for the features you’ve written already behind the feature flag, while holding off on the stuff that doesn’t exist yet. Feature flags don’t mean throwing the baby out with the bathwater.
- moritzwarhier 2y agoShip of theseus in practise. Kind of. Atomic commits are hard.
- bobthepanda 2y agoThis is so common that it is an architecture pattern: https://learn.microsoft.com/en-us/azure/architecture/patterns/strangler-fig https://learn.microsoft.com/en-us/azure/architecture/pattern... The original post describing it: https://martinfowler.com/bliki/StranglerFigApplication.html https://martinfowler.com/bliki/StranglerFigApplication.html
- ahepp 2y agodo you think this can be done at the level of commits in a PR, or is there a big advantage to making it multiple PRs?
- bobthepanda 2y agoIf each commit can be exposed to a separate review then that works. The main issue is humans, 1. Do not want to read long things, slowing down approvals since no one wants to touch it 2. Cannot effectively review long things and something is more likely to slip past.