Improving reviewing and scrutiny

[H. S. Teoh via Digitalmars-d]

On Wed, Feb 04, 2015 at 03:01:47PM -0800, Andrei Alexandrescu via Digitalmars-d wrote:
> I just stepped into a disaster zone in std.file and submitted
> https://issues.dlang.org/show_bug.cgi?id=14125.
> 
> This reveals the merits of reviewing pull requests carefully, and the
> issues that can crop in when that doesn't happen.
> 
> I appeal again to a broader participation to reviewing pull requests
> by the community, even folks who don't have commit rights yet. A good
> review counts a lot, and the lack thereof... well see above.
> 
> Also I'd like to open discussion with the dlang brass to figure out
> ways on how to make sure this doesn't happen again in the future.
[...]

I don't know if there's anything that can prevent this, short of
thorough reviewing before merging.

Perhaps there can be a Phobos Reviewers' Guide that lists some common
mistakes / gotches to watch out for. It won't catch everything, but at
least it can be a preliminary checklist that must pass before a PR is
even considered for merging. Among the items could be:

- Coding style -- I think we've nailed this one pretty good so far, but
  sometimes things still slip through.

- New code that's missing ddoc comments.

- For modules that have doc headers that link to individual functions
  (e.g. std.algorithm.*, std.range.*, etc.), any PR that introduces new
  functions must also update the links in these headers. There have
  already been a number of new functions that got in, but nobody knew
  about them because they were not linked to the doc headers.

- Code that is missing unittests, or isn't adequately covered by
  unittests.

- Use of ddoc'd unittests instead of untested code samples in comments.

- Review ALL usages of @trusted very carefully -- @trusted should not be
  used where avoidable, and even when unavoidable it should have (1)
  ample justification and (2) be confined to a small part of the code,
  usually a small helper function (it's not acceptable for a gigantic
  5-page function to be @trusted -- nobody is going to have the patience
  to review the whole thing every time something changes).

- Avoid module-level public imports except where justified -- scoped
  imports should be preferred. (Though there are gotchas in this area,
  e.g. issue 313; reviewers should get up to speed about how to handle
  this).

Others can add to this list -- I'm sure there are more.

On second thoughts, such a list could be a bit daunting for potential
reviewers -- it could potentially become quite a long list! Perhaps the
correct approach is to use it as a checklist for each PR, and different
reviewers can check off different items on the list, and merging will be
put on hold until at least all items have been checked off.


T

-- 
Chance favours the prepared mind. -- Louis Pasteur