>In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.
It’s a nice rule of thumb for CLs (changelists) that provide a bug fix or refactoring. But I think it’s a terribly useless rule of thumb when evaluating a CL that just implements a new feature.
Imagine some new features such as “support exporting to CSV” or “auto-generate avatars for new users”. Implementing them will necessarily add more code, and more complexity, to the codebase. And adding more complexity makes the code harder to work with – in other words, harms “the overall code health of the system”. Businesses develop new features because of the improvements to the product they bring and the reputational or monetary gains that come from that, in spite of the damage done to the codebase. The only way you could get a new-feature CL to pass by the standard you quoted (the standard of “improving the overall code health of the system being worked on”) is to refactor existing messy code written by someone else, and that would not be sustainable.
Does anyone see a way to interpret the rule, or have a preferred variation of the rule, that makes it useful for evaluating new-feature CLs?
Yeah, that was the first section I read and I raised an eyebrow, I hadn't thought of it like that yet. I can see how one would get paralyzed by a focus on perfection; I too have left idk, dozens of comments on a feature PR of a few hundred lines of code.
Mind you that was before Prettier, nowadays a lot of these niggly things are automatically identified and fixed in editors and a pre-commit hook.
Imagine some new features such as “support exporting to CSV” or “auto-generate avatars for new users”. Implementing them will necessarily add more code, and more complexity, to the codebase. And adding more complexity makes the code harder to work with – in other words, harms “the overall code health of the system”. Businesses develop new features because of the improvements to the product they bring and the reputational or monetary gains that come from that, in spite of the damage done to the codebase. The only way you could get a new-feature CL to pass by the standard you quoted (the standard of “improving the overall code health of the system being worked on”) is to refactor existing messy code written by someone else, and that would not be sustainable.
Does anyone see a way to interpret the rule, or have a preferred variation of the rule, that makes it useful for evaluating new-feature CLs?