I approved DIP1036e

Adam Wilson flyboynw at gmail.com
Sun Jan 21 11:13:41 UTC 2024


On Saturday, 20 January 2024 at 21:11:20 UTC, Walter Bright wrote:
> DMD/Phobos/etc is a large, complex project. We simply cannot 
> afford to accept low quality contributions, as the project will 
> become unmaintainable. Anyone should be proud when their 
> contributions meet our high standards. We're willing to help 
> those who do not get better - but if they don't want help, I'm 
> sorry about that, but that's the way it goes.

I think we need to specify what we mean when we say "low quality 
contributions" as opinions on the subject vary drastically. 
Quality here would be defined, in order, as:
- Correctness (No obvious bugs, unittests pass, etc.)
- Completeness (Does the code cover all know/specified cases)
- Legibility (Can the average engineer understand the code? This 
isn't the Obfuscated C Contest.)

Sadly, because it's easier bikeshed, especially when the reviewer 
doesn't actually understand the code, most code reviews turn into 
nit-picks focusing on sub-optimal but ultimately insignificant 
deviations from the reviewers stylistic preferences, all in the 
name of "legibility". The single best way to derail a 
conversation on code reviews is to bring up the topic of ... line 
length. Many an epic review rant has been launched on the best 
line length. To hear the reviewer tell it, line length violations 
are the equivalent of ritually sacrificing kittens, or some other 
equally heinous crime.

But aside from personal preference and varied screen widths, line 
length is irrelevant to any rational definition of quality. The 
compiler certainly does not care about it, it's a pure human 
preference that makes no difference in the slightest to the 
generated binary. The best argument you can make is that they 
increase the readability of the code, which is at best a minor 
tributary of understanding the code. Yes, organizing the code in 
a more readable format will nominally improve legibility, but 
many Obfuscated C programs have no trouble adhering to the sacred 
80 character line limit. I will (and have frequently in the past) 
merged code that violated the Holy Character Limit, because the 
code was still legible.

Even more importantly, nitpicking greatly increases the noise of 
a code review. We all have a story from our past where we missed 
the one important review item that was drowning in a sea of 
nitpicks. Improve the signal-to-noise ratio, and the quality of 
the code, as defined above, will naturally improve.

However, there is another reason to avoid nitpicking. It improves 
the our relationship with those who conduct the reviews. 
Nitpicking is often used as a tool to stall out or otherwise 
derail a PR that the reviewer either doesn't understand or 
doesn't *want* to understand, usually because they don't want to 
see it merged for whatever reason. And given recent events in D, 
I think this is an important point. One solid and easily 
achievable way to improve our relationships with one another is 
reduce the amount of nitpicking we do of others code. I think we 
forget that the implemented will naturally see review comments 
(critiques) as failures, and seeing a blizzard of review comments 
is incredibly demoralizing.

For a more in depth look at the problems with nitpicking in 
reviews, I highly recommend [this 
article](https://blog.danlew.net/2021/02/23/stop-nitpicking-in-code-reviews/) by Dan Lew.

And yes, if I get a quality PR that has a line of code with 121 
characters, I am merging it, without comment, without hesitation, 
and with a smile on my face, because somebody saw fit to donate 
their time to help me out.


More information about the Digitalmars-d mailing list