Arguing for Consistency

[purpleKarrot]

I recommend the following article to all reviewers and moderators in particular: http://slashslash.info/2018/02/a-foolish-consistency/ (You can stop reading at "East Const". While I share the author's preference for the placement of the const keyword, the reason why I link this article is its introduction).

The scenario that the author describes to have experienced a number of times in his career can be observed in code review comments in Bitcoin Core quite often.

I think it would be beneficial to the quality of the code and also to the quality of reviews if that consistency argument was not considered as a trump card, but as a red flag.

[maflcko]

Maybe I am misunderstanding, but I am pretty sure this is already documented as such:

https://github.com/bitcoin/bitcoin/blob/58a7869f860f022e6163dc3fae3106a0599830c5/doc/developer-notes.md#L5-L11

Of course, every single change is evaluated case-by-case and exact processes may differ case-by-case or even dev-by-dev, but the general ideas are already mentioned and explained in the dev notes, no?

[purpleKarrot]

@maflcko, this is about a review guideline, not a coding guideline. I don't want to list concrete examples because I don't want to blame anyone. But I can make up an example that can be easily translated to many other cases.

Say you have a legacy codebase that uses many iterator-based loops. There is a PR that adds some code and it uses range-based loops (in new code). There is a review comment that argues that iterator-based loops should be preferred for consistency with existing code until a sweeping change of all iterator-based loops can be performed. Or worse, the burden of that sweeping change is put on the author of the PR.

[maflcko]

Well, I think the coding/contrib guideline is also the review guideline (possibly stuff that reviewers should flag during review).

There is a PR that adds some code and it uses range-based loops (in new code). There is a review comment that argues that iterator-based loops should be preferred for consistency

I think a small comment linking to https://github.com/bitcoin/bitcoin/blob/58a7869f860f022e6163dc3fae3106a0599830c5/doc/developer-notes.md#L5-L11 should often resolve the thread.

Of course, every case is different, and the general rules are only guidelines, not to be strictly applied to all cases.

For example, if there are only a few instances of the pattern, it could make sense to just add a commit or PR to sweep it away in one go, instead of arguing for years/months in code review to only update code when touched (and having the overhead of reviewing the replacement every time a line of code is changed). It is good to have the rules as a general framework, but often it makes most sense to evaluate on a case-by-case basis and do "what makes most sense overall" (the meaning of which may differ between devs).