User:icon
My vision for flashprog and quirks as a developer
My primary focus was always to add new features. At first there was a reluctance towards changing existing code (mostly by fear not to get things reviewed). Today, I always try to mix refactorings in when I add something new. Generally, this combination seems to encourage reviewers too. When I see a shiny new feature at the end of a patch train, I know why I’m reviewing.
Parts of flashprog’s codebase are quite old and hard to understand when read for the first time. That doesn’t mean anything is wrong or would even have to be rewritten. Still it is important not to be afraid of refactoring something when it becomes necessary. In the last years, I’ve often only understood prior design choices when I started to work on the code. I would like us to discuss such things ahead of reviewing patches. But also want to encourage changes that ease fixing or enhancing something or adding new features. Sometimes adding something on top without changing existing code may seem easier, but increases the maintenance burden in the long run. It seems important to discuss such aspects before a code review.
Flashprog, more than some other projects, has code areas that change little after the original merge. The individual programmer drivers, for instance. I see this as a feature, author’s can come back after years and still recognize their code. And I feel a deep respect for original authors. This makes it hard for me to agree to treewide changes that merely align things, e.g. change style, identifiers, order of functions or declarations etc. generally, cosmetic changes.
Some generic changes are necessary, of course. Refactorings can help to assist new development. As a reviewer, it’s much easier for me to agree to changes when I can see the bigger picture. When something seems too simple for a discussion ahead of review, finishing a patch train before starting the review can help to understand the bigger picture. But it might be hard to assess if something will make it through review. We need to find some balance between the length of patch trains and discussing things before patches are written. Seeing a long patch train not getting merged can cause sorry feelings for everyone. If in doubt, ask ahead!
Coding style… well, I wouldn’t object to automatically run checkpatch if somebody wants that. At Linux, AFAICT, the rule is to give authors some space, i.e. not to be too pedantic during review (unless it just looks all wrong). I like that. It can make the process more efficient. Also when code is merged, I’d like to just keep things as they are, as long as everything is readable. I know many developers have some light OCD when it comes to reading code and checking if everything is in order. At least I have. But you don’t have to fear me when an #include list looks too ragged. I believe to have found some balance over the years and want to encourage everybody to try the same.