www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Improving reviewing and scrutiny

reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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.


Thanks,

Andrei
Feb 04 2015
next sibling parent reply "Foo" <Foo test.de> writes:
On Wednesday, 4 February 2015 at 23:01:48 UTC, Andrei 
Alexandrescu 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.


 Thanks,

 Andrei


Did you take a deeper look after I grumbled about it? :P
Feb 04 2015
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 2/4/15 3:09 PM, Foo wrote:

 Did you take a deeper look after I grumbled about it? :P


That's exactly right. I was like, "let me show a shiny good example of 
how things are done, how wonderful D is, how..." urgh. -- Andrei
Feb 04 2015
prev sibling next sibling parent "Dicebot" <public dicebot.lv> writes:
On Wednesday, 4 February 2015 at 23:01:48 UTC, Andrei 
Alexandrescu 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.


 Thanks,

 Andrei


Answered in bugzilla
Feb 04 2015
prev sibling next sibling parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
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
Feb 04 2015
prev sibling next sibling parent reply "Jonathan Marler" <johnnymarler gmail.com> writes:
On Wednesday, 4 February 2015 at 23:01:48 UTC, Andrei 
Alexandrescu wrote:

 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.


 Thanks,

 Andrei


Find out who approved the PRs.  Maybe an approver needs to be 
removed or just needs to be told that they need to spend more 
time before merging PRs.  Find out what areas the approvers are 
weak in and let them know they need to improve in those areas.  
Self-introspection as a community would be a very good investment 
in improving the quality of phobos.
Feb 04 2015
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 2/4/15 3:46 PM, Jonathan Marler wrote:

 On Wednesday, 4 February 2015 at 23:01:48 UTC, Andrei Alexandrescu wrote:

 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.


 Thanks,

 Andrei


 Find out who approved the PRs.  Maybe an approver needs to be removed or
 just needs to be told that they need to spend more time before merging
 PRs.  Find out what areas the approvers are weak in and let them know
 they need to improve in those areas. Self-introspection as a community
 would be a very good investment in improving the quality of phobos.


No need to rescind rights, but I do want us to count on more eyes and 
better judgment.

Sometimes I approved stuff! Look at 
https://github.com/D-Programming-Language/phobos/commit/a62bdfe63f472518bc16b
c950311edae9a534b3. 
I assumed it was a simple change, not an application of an idiomn out of 
whack.

I have a practical matter - looking at 
https://github.com/D-Programming-Language/phobos/commits/master/std/file.d 
I can see the commits but not the pull requests they correspond to. Is 
there an easy way to see them?


Andrei
Feb 04 2015
next sibling parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Wed, Feb 04, 2015 at 04:00:59PM -0800, Andrei Alexandrescu via Digitalmars-d
wrote:
[...]

 I have a practical matter - looking at
 https://github.com/D-Programming-Language/phobos/commits/master/std/file.d I
 can see the commits but not the pull requests they correspond to. Is
 there an easy way to see them?


[...]

Try using git log --graph and searching for the offending commit. The
merge commit is usually not far above it, and the commit message should
contain the PR number.


T

-- 
The fact that anyone still uses AOL shows that even the presence of options
doesn't stop some people from picking the pessimal one. - Mike Ellis
Feb 04 2015
prev sibling parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Thursday, 5 February 2015 at 00:01:00 UTC, Andrei Alexandrescu 
wrote:

 I have a practical matter - looking at 
 https://github.com/D-Programming-Language/phobos/commits/master/std/file.d 
 I can see the commits but not the pull requests they correspond 
 to. Is there an easy way to see them?


If you click the commit, under the commit message you'll see:




request.
Feb 04 2015
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 2/4/15 8:40 PM, Vladimir Panteleev wrote:

 On Thursday, 5 February 2015 at 00:01:00 UTC, Andrei Alexandrescu wrote:

 I have a practical matter - looking at
 https://github.com/D-Programming-Language/phobos/commits/master/std/file.d
 I can see the commits but not the pull requests they correspond to. Is
 there an easy way to see them?


 If you click the commit, under the commit message you'll see:






Thanks very much. -- Andrei
Feb 05 2015
prev sibling parent reply "Atila Neves" <atila.neves gmail.com> writes:
I learned a lot about how  trusted is supposed to be used by the 
discussion on that bug report. I don't know about the rest of 
you, but it wasn't obvious to me at all beforehand.

Atila

On Wednesday, 4 February 2015 at 23:01:48 UTC, Andrei 
Alexandrescu 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.


 Thanks,

 Andrei
Feb 05 2015
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 2/5/15 5:02 AM, Atila Neves wrote:

 I learned a lot about how  trusted is supposed to be used by the
 discussion on that bug report. I don't know about the rest of you, but
 it wasn't obvious to me at all beforehand.


Suggestion: write a blog post about it that distills the main points, 
with code samples and git history, the works. I think a lot of folks 
would find it interesting. -- Andrei
Feb 05 2015