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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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