Re: Phobos PR in need of review/merge
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=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=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.
Re: Phobos PR in need of review/merge
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=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=is%3Apr%20review%3Anone%20is%3Aopen%20-label%3A%22needs%20work%22%20 The "needs work" label gets automatically removed on a new push.
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
On Sunday, July 02, 2017 00:04:40 H. S. Teoh via Digitalmars-d wrote: > On Sun, Jul 02, 2017 at 01:56:22AM +, 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
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
On Sun, Jul 02, 2017 at 01:56:22AM +, 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.
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
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 :|
Re: Phobos PR in need of review/merge
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?
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
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
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
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
Re: Phobos PR in need of review/merge
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).
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
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
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
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?
Re: Phobos PR in need of review/merge
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
Re: Phobos PR in need of review/merge
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
Re: Phobos PR in need of review/merge
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.
Re: Phobos PR in need of review/merge
Another PR that somebody in IRC mentioned: https://github.com/dlang/phobos/pull/5004 This one hasn't had any response for awhile either.
Phobos PR in need of review/merge
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
