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