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

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

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

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

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

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.

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/a62bdfe63f472518bc16bbc950311edae9a534b3. 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

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

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:

master (#1234)

#1234 is the pull request #, you can click it to get to the pull request.

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:
>
> master (#1234)
>
> #1234 is the pull request #, you can click it to get to the pull request.

Thanks very much. -- Andrei