www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Phobos PR in need of review/merge

reply Meta <jared771 gmail.com> writes:
Recently, a pull request was closed out of frustration by the 
submitter: https://github.com/dlang/phobos/pull/5309

I have asked them to re-submit the PR[1] in question so we can 
try to get it through. It's a mostly trivial change that could do 
with some eyes, comments, and most importantly, somebody with 
merge privileges to actually merge it.

On this topic, I feel like we've been falling behind lately in 
responding to PRs promptly, communicating with submitters on what 
changes (if any) are needed to get the PR into merge shape, and 
actually getting stuff merged (this isn't anything new of 
course). I don't have the data to back me up yet, but I am going 
to try to gather what I can and make a post about it sometime 
within the next month. Any ideas or insights are welcome.

1. https://github.com/dlang/phobos/pull/5515
Jun 26 2017
next sibling parent reply Meta <jared771 gmail.com> writes:
Another PR that somebody in IRC mentioned: 
https://github.com/dlang/phobos/pull/5004

This one hasn't had any response for awhile either.
Jun 26 2017
parent Meta <jared771 gmail.com> writes:
On Tuesday, 27 June 2017 at 01:49:52 UTC, Meta wrote:

 Another PR that somebody in IRC mentioned: 
 https://github.com/dlang/phobos/pull/5004

 This one hasn't had any response for awhile either.


The first PR has been reviewed and merged thanks to Jack Stouffer 
and Wilzbach. Can anyone familiar with std.concurrency review 
this one?
Jun 30 2017
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 6/26/2017 6:35 PM, Meta wrote:

 Recently, a pull request was closed out of frustration by the submitter: 
 https://github.com/dlang/phobos/pull/5309
 
 I have asked them to re-submit the PR[1] in question so we can try to get it 
 through. It's a mostly trivial change that could do with some eyes, comments, 
 and most importantly, somebody with merge privileges to actually merge it.
 
 On this topic, I feel like we've been falling behind lately in responding to
PRs 
 promptly, communicating with submitters on what changes (if any) are needed to 
 get the PR into merge shape, and actually getting stuff merged (this isn't 
 anything new of course). I don't have the data to back me up yet, but I am
going 
 to try to gather what I can and make a post about it sometime within the next 
 month. Any ideas or insights are welcome.
 
 1. https://github.com/dlang/phobos/pull/5515


You can also specifically request a review from one of Team Phobos:

   https://github.com/orgs/dlang/teams/team-phobos/members

Just click on the [Reviwers] link.
Jun 26 2017
parent reply Mike Wey <mike-wey example.com> writes:
On 27-06-17 08:49, Walter Bright wrote:

 You can also specifically request a review from one of Team Phobos:
 
 https://github.com/orgs/dlang/teams/team-phobos/members
 
 Just click on the [Reviwers] link.


Is that page private? I get an 404 error, and i can't find the page on 
github.

-- 
Mike Wey
Jun 27 2017
next sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 6/27/17 1:21 PM, Mike Wey wrote:

 On 27-06-17 08:49, Walter Bright wrote:

 You can also specifically request a review from one of Team Phobos:

 https://github.com/orgs/dlang/teams/team-phobos/members

 Just click on the [Reviwers] link.


 
 Is that page private? I get an 404 error, and i can't find the page on 
 github.
 


Yes, it's private. It's actually hard to figure out how github looks 
from a non-member account when you are a member, because you need a 
separate github login.

I think you can request a review though pretty easily on a PR by 
clicking on the "Reviewers" link (has a little gear next to it) on the 
upper right.

-Steve
Jun 27 2017
parent reply Seb <seb wilzba.ch> writes:
On Tuesday, 27 June 2017 at 19:15:46 UTC, Steven Schveighoffer 
wrote:

 On 6/27/17 1:21 PM, Mike Wey wrote:

 On 27-06-17 08:49, Walter Bright wrote:

 You can also specifically request a review from one of Team 
 Phobos:

 https://github.com/orgs/dlang/teams/team-phobos/members

 Just click on the [Reviwers] link.


 
 Is that page private? I get an 404 error, and i can't find the 
 page on github.
 


 Yes, it's private.


However, the member list of an organization is public:

https://github.com/orgs/dlang/people
https://api.github.com/orgs/dlang/public_members?per_page=100

Note that:
- people need to actively set their name to public (so that 
everyone is part of this list)
- there is no association to repos and


 It's actually hard to figure out how github looks from a 
 non-member account when you are a member, because you need a 
 separate github login.


Private/In-cognito tab?


 I think you can request a review though pretty easily on a PR 
 by clicking on the "Reviewers" link (has a little gear next to 
 it) on the upper right.


Unfortunately only GH members can do this - though we should 
increase the team size here (and thus have more people who can 
help out).

Btw in case you didn't know this, you can ping _everyone_ of Team 
Phobos easily with  dlang/team-phobos (though I think only GH 
members can do this).
Jun 27 2017
parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 6/27/17 3:54 PM, Seb wrote:

 On Tuesday, 27 June 2017 at 19:15:46 UTC, Steven Schveighoffer wrote:

 It's actually hard to figure out how github looks from a non-member 
 account when you are a member, because you need a separate github login.


 
 Private/In-cognito tab?


github doesn't always work or have all the UI available unless you are 
logged in. I don't have a non-dlang login to github, so I can't see what 
it looks like for others.

-Steve
Jun 27 2017
prev sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 6/27/2017 10:21 AM, Mike Wey wrote:

 On 27-06-17 08:49, Walter Bright wrote:

 You can also specifically request a review from one of Team Phobos:

 https://github.com/orgs/dlang/teams/team-phobos/members

 Just click on the [Reviwers] link.


 
 Is that page private? I get an 404 error, and i can't find the page on github.


Here's the list:

burner
repeatedly
klickverbot
braddr
alexrp
jakobovrum
cybershadow
kyllingstad
dmitryolschansky
martinnowak
ibuclaw
quickfur
andralex
walterbright
jmdavis
yebblies
schveiguy
jackstouffer
rainers
9il
hackerpilot
uplinkcoder
zombiedev
wilzbach
andrewedwards
andrejmitrovic
Jun 27 2017
prev sibling next sibling parent reply Seb <seb wilzba.ch> writes:
On Tuesday, 27 June 2017 at 01:35:31 UTC, Meta wrote:

 I don't have the data to back me up yet, but I am going to try 
 to gather what I can and make a post about it sometime within 
 the next month. Any ideas or insights are welcome.


Recently [1] I deployed the first iteration of a daily cronjob 
for the Dlang-Bot that does auto-labeling of (a) "stalled" (=no 
activity in 60 days), (b) "needs rebase" (=merge conflicts) and 
(c) "needs work" (=two or more failing CIs).
It's far from perfect, but the idea is to have a better detection 
for stalled issues (see e.g. [2]) and then add better automation 
to them. For example, pinging reviewers automatically if an issue 
gets stalled and maybe even automatically fixing some (e.g. 
trivial merge conflicts).


[1] https://github.com/dlang-bots/dlang-bot/pull/52
[2] 
https://github.com/dlang/phobos/pulls?q=is%3Aopen+is%3Apr+label%3Astalled
[3] https://github.com/dlang-bots/dlang-bot/issues/47
Jun 27 2017
parent Dukc <ajieskola gmail.com> writes:
On Tuesday, 27 June 2017 at 07:07:02 UTC, Seb wrote:

 Recently [1] I deployed the first iteration of a daily cronjob 
 for the Dlang-Bot


Good idea, definitely worth trying.
Jun 27 2017
prev sibling next sibling parent reply Dukc <ajieskola gmail.com> writes:
On Tuesday, 27 June 2017 at 01:35:31 UTC, Meta wrote:

 Recently, a pull request was closed out of frustration by the 
 submitter: https://github.com/dlang/phobos/pull/5309


Topic reporting for duty :)

I have no hard feelings about this... I just want to collect my 
garbage when my requests stall. I understand good coders are 
often too busy to go trough all waiting requests thoughtfully, 
and it's ok if something slips past. But there is just no reason 
I see to keep a request in "alive" state if I don't check it 
actively anymore. The closed pr can be opened later if I or 
someone else wishes to push for it again.

I think the issue here is that I don't remind often enough that 
I'm still waiting, or I do it at wrong place. What is the 
convention here?
Jun 27 2017
next sibling parent Meta <jared771 gmail.com> writes:
On Tuesday, 27 June 2017 at 18:09:01 UTC, Dukc wrote:

 I think the issue here is that I don't remind often enough that 
 I'm still waiting, or I do it at wrong place. What is the 
 convention here?


The convention is to spend a lot of time waiting and pinging 
people until it finally gets merged. I think we can all agree 
that this approach really doesn't scale.
Jun 27 2017
prev sibling parent reply Brad Roberts via Digitalmars-d <digitalmars-d puremagic.com> writes:
On 6/27/17 11:09 AM, Dukc via Digitalmars-d wrote:


 But there is just no reason I see to keep a request in 
 "alive" state if I don't check it actively anymore. The closed pr can be 
 opened later if I or someone else wishes to push for it again.


There's a very good reason to leave requests open:  a closed request is 
gone, never to be seen again (yes, it's technically still findable if 
you search closed but not merged requests, but it's super unlikely to 
ever happen).  An open request, though one among many, is still 
discoverable.  It's in the list and eventually someone will ping it or 
stumble on it, or maybe one dream day there will be sufficient activity 
to chip away at the massive (350ish right now) queue of pulls and it'll 
be taken care of.
Jun 27 2017
parent reply Dukc <ajieskola gmail.com> writes:
On Tuesday, 27 June 2017 at 19:40:52 UTC, Brad Roberts wrote:

 There's a very good reason to leave requests open:  a closed 
 request is gone, never to be seen again.


Well explained. So I quess that next time I should just leave a 
post there that I'm abandoning it. After many pings and a 
notification at forum of course.
Jun 27 2017
parent Walter Bright <newshound2 digitalmars.com> writes:
On 6/27/2017 2:39 PM, Dukc wrote:

 On Tuesday, 27 June 2017 at 19:40:52 UTC, Brad Roberts wrote:

 There's a very good reason to leave requests open:  a closed request is gone, 
 never to be seen again.


 
 Well explained. So I quess that next time I should just leave a post there
that 
 I'm abandoning it. After many pings and a notification at forum of course.


Abandoning it is fine. But please don't close good work! Brad is right. The
time 
may not be right for it yet, but closing it means it will never be looked at
again.
Jun 28 2017
prev sibling next sibling parent notna <notna.remove.this ist-einmalig.de> writes:
On Tuesday, 27 June 2017 at 01:35:31 UTC, Meta wrote:

 Recently, a pull request was closed out of frustration by the 
 submitter: https://github.com/dlang/phobos/pull/5309

 I have asked them to re-submit the PR[1] in question so we can 
 try to get it through. It's a mostly trivial change that could 
 do with some eyes, comments, and most importantly, somebody 
 with merge privileges to actually merge it.

 On this topic, I feel like we've been falling behind lately in 
 responding to PRs promptly, communicating with submitters on 
 what changes (if any) are needed to get the PR into merge 
 shape, and actually getting stuff merged (this isn't anything 
 new of course). I don't have the data to back me up yet, but I 
 am going to try to gather what I can and make a post about it 
 sometime within the next month. Any ideas or insights are 
 welcome.

 1. https://github.com/dlang/phobos/pull/5515


"closed out of frustration" reminds me on one of my 2-3 pull 
requests some time ago - https://github.com/dlang/phobos/pull/3601

If I remember correct, I've just copied one of the already 
existing "Example" entries in that D module and added some text 
PLUS the the forum link... and later was surprised about the 
upcoming discussion where an example should go

Btw., dfmt (or what ever the tools name was/is), did not compile 
on my Windows box at that time. Not sure if if would have done 
the "blank line" and "Example goes somewhere else" magic the 
right/desired way?!

PS., don't get me wrong, no blaming here, but I had no intention 
to learn Git or Github and just thought it would be a pity if the 
given example by Vladimir, which btw at that time saved my day, 
would be lost in the dark holes of the internet ;)

PPS., never made a pull again and keep all the found forum 
treasures in a private places since then :|
Jul 01 2017
prev sibling next sibling parent reply Jack Stouffer <jack jackstouffer.com> writes:
On Tuesday, 27 June 2017 at 01:35:31 UTC, Meta wrote:

 On this topic, I feel like we've been falling behind lately in 
 responding to PRs promptly, communicating with submitters on 
 what changes (if any) are needed to get the PR into merge 
 shape, and actually getting stuff merged (this isn't anything 
 new of course). I don't have the data to back me up yet, but I 
 am going to try to gather what I can and make a post about it 
 sometime within the next month. Any ideas or insights are 
 welcome.

 1. https://github.com/dlang/phobos/pull/5515


This is particularly my fault. I haven't been reviewing PRs 
nearly as much as I was in the past.

I think the PR queue on Phobos has gotten out of control. Ideally 
we'd have around 50 or less PRs open.

I'm going to try to comment on/review at least one PR a day from 
here on out to slowly chip away at the queue.
Jul 01 2017
next sibling parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Sun, Jul 02, 2017 at 01:56:22AM +0000, Jack Stouffer via Digitalmars-d wrote:

 On Tuesday, 27 June 2017 at 01:35:31 UTC, Meta wrote:

 On this topic, I feel like we've been falling behind lately in
 responding to PRs promptly, communicating with submitters on what
 changes (if any) are needed to get the PR into merge shape, and
 actually getting stuff merged (this isn't anything new of course). I
 don't have the data to back me up yet, but I am going to try to
 gather what I can and make a post about it sometime within the next
 month. Any ideas or insights are welcome.
 
 1. https://github.com/dlang/phobos/pull/5515


 
 This is particularly my fault. I haven't been reviewing PRs nearly as
 much as I was in the past.


I don't think it's specifically your fault... I think we're just short
of hands on team Phobos.  I used to do a lot more reviewing / merging
too, but this past year I've been unable to because now I have a young
child and free time is a luxury I often don't have anymore.  We really,
really, need more hands on board to handle the amount of PRs that are
coming in, especially now that D is getting wider exposure and
attracting more contributors.



 I think the PR queue on Phobos has gotten out of control. Ideally we'd
 have around 50 or less PRs open.


Yeah it has. :-(  When I was first given commit access to Phobos,
Dicebot & myself & a few others worked hard to cut the queue from around
70-80 (IIRC) down to about 30-35.  Our goal was to cut it down to 25 (1
page on Github).  Unfortunately, Dicebot left for various reasons, and I
got too busy, so the queue has clogged back up and now has overflowed
past where it used to be.



 I'm going to try to comment on/review at least one PR a day from here
 on out to slowly chip away at the queue.


Thanks, that is greatly needed!  I'll try to check more often, but free
time is just so hard to find these days...

But ultimately, what we desperately need is more hands on team Phobos.
The workload is far more than 1 person can handle in the long run. Or,
for that matter, for a small team of the current size to handle.  The
last thing we want to happen is for you (or anyone else) to burn out and
give up, then we'll be back to square one again (or worse).


T

-- 
What's a "hot crossed bun"? An angry rabbit.
Jul 02 2017
parent Dukc <ajieskola gmail.com> writes:
I see that my work has now been merged. Thanks for the effort 
everyone.

On Sunday, 2 July 2017 at 07:04:40 UTC, H. S. Teoh wrote:

 Our goal was to cut it down to 25


I don't think the amount of open work is the problem. The metrics 
that matter IMO:

1. The time and likelihood to get a review after submitting a pr 
or completing changes requested to it. If the pr stalls because 
of the submitter, that does not matter. But if the tests pass and 
the reviewer has addressed all concerns, the less he has to push 
the reviewers the better.

2. Times one has to make changes before having the work accepted. 
The pr should not have to be perfect to get merged, but of course 
it must break nothing unless it's essential.

3. The likelihood of the work getting accepted at all, if pushed 
to the final decision. I think the state of that is good already: 
I have got nothing rejected what wasn't because I myself 
overlooked something.
Jul 02 2017
prev sibling parent Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Sunday, July 02, 2017 00:04:40 H. S. Teoh via Digitalmars-d wrote:

 On Sun, Jul 02, 2017 at 01:56:22AM +0000, Jack Stouffer via Digitalmars-d 


wrote:

 On Tuesday, 27 June 2017 at 01:35:31 UTC, Meta wrote:

 On this topic, I feel like we've been falling behind lately in
 responding to PRs promptly, communicating with submitters on what
 changes (if any) are needed to get the PR into merge shape, and
 actually getting stuff merged (this isn't anything new of course). I
 don't have the data to back me up yet, but I am going to try to
 gather what I can and make a post about it sometime within the next
 month. Any ideas or insights are welcome.

 1. https://github.com/dlang/phobos/pull/5515


 This is particularly my fault. I haven't been reviewing PRs nearly as
 much as I was in the past.


 I don't think it's specifically your fault... I think we're just short
 of hands on team Phobos.  I used to do a lot more reviewing / merging
 too, but this past year I've been unable to because


[...]

I think that that's the story for a number of us. Several years ago, I was
reviewing and merging more PRs than anyone, but the past couple of years,
I've done relatively little reviewing. I just have a hard time finding time
to spend on this sort of thing. And this year, the time since dconf has been
particularly bad for me. Some of us do need to find a way to spend more time
on this sort of thing, but overall, we simply need more qualified people who
have the time and inclination to review PRs. Too often, the qualified folks
with the time to do it don't continue to have the that kind of time.

- Jonathan M Davis
Jul 03 2017
prev sibling parent reply Meta <jared771 gmail.com> writes:
I thought I'd let everyone know that there has been a whopping 36 
PRs merged in the past week (versus 17 opened). We're now sitting 
at 114 open Phobos PRs. Thanks to the reviewers/mergers who put 
in the effort to get that number down.
Jul 07 2017
parent reply Seb <seb wilzba.ch> writes:
On Saturday, 8 July 2017 at 06:05:54 UTC, Meta wrote:

 I thought I'd let everyone know that there has been a whopping 
 36 PRs merged in the past week (versus 17 opened). We're now 
 sitting at 114 open Phobos PRs. Thanks to the reviewers/mergers 
 who put in the effort to get that number down.


Btw _every_ helping hand in reviewing PRs is very welcome.
It's not very difficult and usually just a "I reviewed this PR 
and it LGTM" helps to bump the priority.
Otherwise of course, the author should be notified about existing 
blocking points in his PR.

Since a couple of months, GitHub allows to list all PRs that 
haven't received a review (yet):

https://github.com/dlang/phobos/pulls?page=3&q=is%3Apr+is%3Aopen+review%3Anone

Also, you can filter out labelled PRs, e.g. all PRs except those 
that depend on work from the submitter:

https://github.com/dlang/phobos/pulls?utf8=%E2%9C%93&q=is%3Apr%20review%3Anone%20is%3Aopen%20-label%3A%22needs%20work%22%20

The "needs work" label gets automatically removed on a new push.
Jul 09 2017
parent Meta <jared771 gmail.com> writes:
On Sunday, 9 July 2017 at 23:05:05 UTC, Seb wrote:

 On Saturday, 8 July 2017 at 06:05:54 UTC, Meta wrote:

 I thought I'd let everyone know that there has been a whopping 
 36 PRs merged in the past week (versus 17 opened). We're now 
 sitting at 114 open Phobos PRs. Thanks to the 
 reviewers/mergers who put in the effort to get that number 
 down.


 Btw _every_ helping hand in reviewing PRs is very welcome.
 It's not very difficult and usually just a "I reviewed this PR 
 and it LGTM" helps to bump the priority.
 Otherwise of course, the author should be notified about 
 existing blocking points in his PR.


Yes, 100%. Even if you feel like you don't have the "authority" 
to review a PR, do it anyway. Even if you don't have merge 
privileges, the more eyes there are on a PR the larger the chance 
of somebody seeing something and pointing it out. It also makes 
it easier for people who *do* have merge privileges if they see 
that it's already been looked over by a few different people.


 Since a couple of months, GitHub allows to list all PRs that 
 haven't received a review (yet):

 https://github.com/dlang/phobos/pulls?page=3&q=is%3Apr+is%3Aopen+review%3Anone

 Also, you can filter out labelled PRs, e.g. all PRs except 
 those that depend on work from the submitter:

 https://github.com/dlang/phobos/pulls?utf8=%E2%9C%93&q=is%3Apr%20review%3Anone%20is%3Aopen%20-label%3A%22needs%20work%22%20

 The "needs work" label gets automatically removed on a new push.


Thanks for getting some more sophisticated automated stuff set 
up. The more small things we can offload onto machines who don't 
mind the tedium and don't forget, the better.
Jul 11 2017