2 ms·
Just wanted to say thanks for sharing these rules, I think they’re super helpful. Do you have any particular rules about performance? I worry that accepting n
by caffeine 4y ago
Just wanted to say thanks for sharing these rules, I think they’re super helpful.
Do you have any particular rules about performance? I worry that accepting net improvements on functionality alone could result in “death-by-a-thousand-cuts” performance issues down the line (assume it’s a project where performance is one of the deliverables).
Would you ask a contributor to perform benchmarks or some other performance validation, for example?
- gsliepen 4y agoIt would depend on the project. If performance is important, it should indeed by validated, but then it would greatly help if the project also has a performance test suite that contributors could run locally, and compare their change against the baseline, and a reviewer could then also verify it themselves. What you do with the performance results really depends on the kind of patch they want to commit. Is it an important bug fix? Then I would get that merged first and worry about fixing performance afterwards. And then you have those patches that improve performance in one area at the cost of decreasing it in another. In that case you might have to use your experience in what is more important to decide whether to accept or reject it.