www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - New Github Issues

reply "Brad Anderson" <eco gnuk.net> writes:
Github updated their Issues system (which includes Pull 
Requests). You can read about it here:

https://github.com/blog/1866-the-new-github-issues

Bugzilla is here to stay but the newly added Labels feature could 
probably help organize Pull Requests. The usual "Enhancement", 
"Bug", etc. are nice but I'm wondering if anyone can think of a 
way they could be used to help deal with the pull request 
backlog. A lot of the time it seems like there is confusion over 
whose court the ball is in. Maybe a "Changes Requested" label to 
show the pull request is waiting on the submitter to make some 
changes reviewers requested. A "Stalled" label could be used when 
the submitter isn't responding to review requests (which would 
eventually result in the pull request being closed).

I think assigning a state to pull requests (rather than just 
reading over comments) would help make the process more clear and 
make the status of pull requests noticeable at a glance rather 
than having to open each one to see what's going on.
Jul 28 2014
next sibling parent "Brad Anderson" <eco gnuk.net> writes:
On Monday, 28 July 2014 at 17:31:56 UTC, Brad Anderson wrote:

 [...]
 A lot of the time it seems like there is confusion over whose
 court the ball is in.


I always forget idioms probably aren't a good idea in an 
international community like this one.

http://idioms.thefreedictionary.com/the+ball+is+in+court


 [...]
Jul 28 2014
prev sibling next sibling parent reply "Daniel Murphy" <yebbliesnospam gmail.com> writes:
"Brad Anderson"  wrote in message 
news:vtizuccrwzyidddfzgbr forum.dlang.org...


 Github updated their Issues system (which includes Pull Requests). You can 
 read about it here:

 https://github.com/blog/1866-the-new-github-issues


Oh FINALLY!!!

I've wanted to use assignment and labels for a long long time but it wasn't 
usable without issue enabled on the repository.


 I think assigning a state to pull requests (rather than just reading over 
 comments) would help make the process more clear and make the status of 
 pull requests noticeable at a glance rather than having to open each one 
 to see what's going on.


By the look of it only people with commit access can change labels, so we 
could also use the to control the auto-tester's auto-merge.
Jul 28 2014
parent reply "Daniel Murphy" <yebbliesnospam gmail.com> writes:
"Daniel Murphy"  wrote in message news:lr6249$18pf$1 digitalmars.com... 


 Oh FINALLY!!!


I've just labelled all the DDMD pull requests.
Jul 28 2014
next sibling parent "Brad Anderson" <eco gnuk.net> writes:
On Monday, 28 July 2014 at 18:23:06 UTC, Daniel Murphy wrote:

 "Daniel Murphy"  wrote in message 
 news:lr6249$18pf$1 digitalmars.com...


 Oh FINALLY!!!


 I've just labelled all the DDMD pull requests.


Nice.

https://github.com/D-Programming-Language/dmd/pulls?q=is%3Aopen+is%3Apr+label%3ADDMD

Way better than clicking through page after page and using Ctrl-F 
to find those.
Jul 28 2014
prev sibling parent "Joseph Cassman" <jc7919 outlook.com> writes:
On Monday, 28 July 2014 at 18:23:06 UTC, Daniel Murphy wrote:

 "Daniel Murphy"  wrote in message 
 news:lr6249$18pf$1 digitalmars.com...


 Oh FINALLY!!!


 I've just labelled all the DDMD pull requests.


This helps understand where the work is going on.
Thanks

Joseph
Jul 28 2014
prev sibling next sibling parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Mon, Jul 28, 2014 at 05:31:55PM +0000, Brad Anderson via Digitalmars-d wrote:

 Github updated their Issues system (which includes Pull Requests). You
 can read about it here:
 
 https://github.com/blog/1866-the-new-github-issues
 
 Bugzilla is here to stay but the newly added Labels feature could
 probably help organize Pull Requests. The usual "Enhancement", "Bug",
 etc. are nice but I'm wondering if anyone can think of a way they
 could be used to help deal with the pull request backlog.


[...]

We could adopt what we did with Phobos PRs over the last 2-3 weeks. :-)

After a rant about the PR backlog in one of those interminable rant
threads, a few of us decided to stop arguing about it and instead do
something about it: ping PRs that haven't been updated for a long time
(changing the sorting to 'least recently updated' or 'oldest' helps find
these stagnating PRs more easily), review PRs that have been sitting
around with no review comments, suggest PRs be closed if there's no hope
they will be merged, etc..  A few committers got a bit more aggressive
about pulling PRs -- with the view that if something was a mistake, we
could always revert it later, since after all that's what git is for!

The result was that the original Phobos PR backlog of about 90+ or so
dropped to around 70 by the end of the week, and this morning we broke
the 60 mark, and we're now down to 56. The PR list page used to be 4
pages, and now it's down to 3 and approaching 2. If this trend will
continue, we should soon be able to get things down to about 1 page or
so, and the situation will be much more controllable.

There was at least 1 revert that I know of from this recent effort, and
1 fixup PR to repair some flaws in a previous PR. But I see that as a
good thing: we're getting things moving, getting the feedback and
discussion going, and making progress in general, instead of waiting
around for PRs to become ideal flawless ivory towers that also never
materialize.

In contrast, the DMD PR list has breached 3 digits, and shows little
sign of slowing down. Perhaps we should consider adopting a similar
approach there as well? ;-)

On that note, though, the Phobos situation, while much improved, still
isn't quite there yet. I'd like to invite everyone here to chime in and
review PRs. As long as you can code in D, you're qualified to review
*something* -- not every PR involves rocket science. Sometimes even
trivial nitpick comments like minor Phobos style violations may help
awaken a dormant PR and get things moving again. You don't need commit
rights to contribute in this way -- I don't, for example -- but your
work will be much appreciated. And I have to say that it is very
gratifying to see the Phobos open PR count drop, and realize I was a
part of it.


T

-- 
The best compiler is between your ears. -- Michael Abrash
Jul 28 2014
prev sibling next sibling parent reply Orvid King <blah38621 gmail.com> writes:
On 7/28/2014 12:31 PM, Brad Anderson wrote:

 Github updated their Issues system (which includes Pull Requests). You
 can read about it here:

 https://github.com/blog/1866-the-new-github-issues

 Bugzilla is here to stay but the newly added Labels feature could
 probably help organize Pull Requests. The usual "Enhancement", "Bug",
 etc. are nice but I'm wondering if anyone can think of a way they could
 be used to help deal with the pull request backlog. A lot of the time it
 seems like there is confusion over whose court the ball is in. Maybe a
 "Changes Requested" label to show the pull request is waiting on the
 submitter to make some changes reviewers requested. A "Stalled" label
 could be used when the submitter isn't responding to review requests
 (which would eventually result in the pull request being closed).

 I think assigning a state to pull requests (rather than just reading
 over comments) would help make the process more clear and make the
 status of pull requests noticeable at a glance rather than having to
 open each one to see what's going on.


How accessible is it possible to make the labels? Can they be made 
semi-publicaly editable like bugzilla's? Also, could we default to 
having an awaiting review label for new PRs? Perhaps even have them 
sorted to be at the top by default?
Jul 28 2014
parent reply "Dicebot" <public dicebot.lv> writes:
On Tuesday, 29 July 2014 at 01:35:30 UTC, Orvid King wrote:

 How accessible is it possible to make the labels?


As far as I can see one needs admin access to the repo to define 
new labels. Merge access is not enough - at least I don't seem to 
be able define new label for Phobos.


 Can they be made semi-publicaly editable like bugzilla's?


Most likely it is same as with issue labels (when you have GitHub 
issues enabled) - admin can define set of labels, anyone with 
merge access can add labels from the list to pull requests, 
everyone else can only view labels and search based on them.


 Also, could we default to having an awaiting review label for 
 new PRs? Perhaps even have them sorted to be at the top by 
 default?


All pull request are awaiting review by default ;) We should 
instead have labels marking those that can't be reviewed for some 
reason - being it a decision block, pending dependency merge or 
missing author.
Jul 28 2014
parent reply "Daniel Murphy" <yebbliesnospam gmail.com> writes:
"Dicebot"  wrote in message news:dltkkijmuwhjcchejjxd forum.dlang.org...


 As far as I can see one needs admin access to the repo to define new 
 labels. Merge access is not enough - at least I don't seem to be able 
 define new label for Phobos.


No, you just need commit access.  On the pull requests page, there is a 
'labels' tab between 'Pull requests' and 'Milestones'.


 Most likely it is same as with issue labels (when you have GitHub issues 
 enabled) - admin can define set of labels, anyone with merge access can 
 add labels from the list to pull requests, everyone else can only view 
 labels and search based on them.


Yep looks right except for creation.


 All pull request are awaiting review by default ;) We should instead have 
 labels marking those that can't be reviewed for some reason - being it a 
 decision block, pending dependency merge or missing author.


You can search for unlabeled pull requests.
Jul 28 2014
parent "Dicebot" <public dicebot.lv> writes:
On Tuesday, 29 July 2014 at 06:24:26 UTC, Daniel Murphy wrote:

 "Dicebot"  wrote in message 
 news:dltkkijmuwhjcchejjxd forum.dlang.org...


 As far as I can see one needs admin access to the repo to 
 define new labels. Merge access is not enough - at least I 
 don't seem to be able define new label for Phobos.


 No, you just need commit access.  On the pull requests page, 
 there is a 'labels' tab between 'Pull requests' and 
 'Milestones'.


Ah new shiny interface, thanks, see it now.
Jul 29 2014
prev sibling parent reply "Dicebot" <public dicebot.lv> writes:
Heh, have written a GitHub feature request about this ~2 weeks 
ago, got answer "we are already working on this, going to be 
release soon". Have been eagerly awaiting announcement since then.

This can be very useful for improving pull request review 
process. I think we should not duplicate PR labels based on 
metadata from bugzilla and instead focus on traits useful for 
actual reviewing and PR categorizing (not related issue 
categorizing).

"DDMD" label for dmd repo is one good example - it does not have 
linked issues and need to be easily queried because of high 
priority of DDMD.

One useful label I can imagine for both DMD and Phobos repos is 
"need-decision" that will mark pull requests blocked until 
someone with authority decides if actual semantics of a change 
are to be accepted or rejected. That way Andrei and Walter will 
have a quick list of pull requests to pay special attention to 
(that no one else can proceed with)

At the same time I recommend avoid creation of many arbitrary 
label tags as those are only useful for searching pull requests 
if you know what labels to look for. Pure description tags give 
nothing over few lines of comments in PR text.
Jul 28 2014
parent reply "Daniel Murphy" <yebbliesnospam gmail.com> writes:
"Dicebot"  wrote in message news:krxuctciwangfhiphava forum.dlang.org...


 One useful label I can imagine for both DMD and Phobos repos is 
 "need-decision" that will mark pull requests blocked until someone with 
 authority decides if actual semantics of a change are to be accepted or 
 rejected. That way Andrei and Walter will have a quick list of pull 
 requests to pay special attention to (that no one else can proceed with)


I'm thinking along the lines of:
- Needs review
- Needs work
- Needs enhancement approval

Now that we actually get a list pull requests have been assigned to a 
certain user, it makes sense to use that feature too.
Jul 28 2014
parent reply "Dicebot" <public dicebot.lv> writes:
On Tuesday, 29 July 2014 at 06:32:15 UTC, Daniel Murphy wrote:

 I'm thinking along the lines of:
 - Needs review
 - Needs work


Makes sense if we agree to add those only if nothing happens with 
pull for relatively long time - otherwise it means lot of useless 
routine of switching back and forth between "needs review" and 
"needs work".


 - Needs enhancement approval


Reason why I have named it "needs-decision" (can't have 
whitespaces in labels afaik) is that it is not necessarily an 
enhancement stuff - sometimes bug fixes
can be also very controversial.


 Now that we actually get a list pull requests have been 
 assigned to a certain user, it makes sense to use that feature 
 too.


Assignment does not really mean much for pure pull requests, in 
absence of issue tracking - only for rare cases when review is 
needed by certain qualified person.

What is more important, in my opinion, is that now we can use 
Milestones for grouping release regression fixes instead of 
encoding that info in title and doing "ping  AndrewEdwards" all 
the time.
Jul 28 2014
parent reply "Daniel Murphy" <yebbliesnospam gmail.com> writes:
"Dicebot"  wrote in message news:efrstwsylpujuyycwcgs forum.dlang.org...


 Makes sense if we agree to add those only if nothing happens with pull for 
 relatively long time - otherwise it means lot of useless routine of 
 switching back and forth between "needs review" and "needs work".


Yeah, I just want an easy way of knowing which pulls are not worth looking 
at, without having to open each one and read the comments to get the status. 
If a pull is marked as needs-review and the autotester is passing, it's 
probably


 Reason why I have named it "needs-decision" (can't have whitespaces in 
 labels afaik) is that it is not necessarily an enhancement stuff - 
 sometimes bug fixes
 can be also very controversial.


'Walter-blocked' is the true meaning.


 Assignment does not really mean much for pure pull requests, in absence of 
 issue tracking - only for rare cases when review is needed by certain 
 qualified person.


I wish they were rare.  In dmd at least.

What is more important, in my opinion, is that now we can use

 Milestones for grouping release regression fixes instead of encoding that 
 info in title and doing "ping  AndrewEdwards" all the time.


This is probably a good idea.
Jul 29 2014
parent reply "Dicebot" <public dicebot.lv> writes:
On Tuesday, 29 July 2014 at 07:18:37 UTC, Daniel Murphy wrote:

 Reason why I have named it "needs-decision" (can't have 
 whitespaces in labels afaik) is that it is not necessarily an 
 enhancement stuff - sometimes bug fixes
 can be also very controversial.


 'Walter-blocked' is the true meaning.


It is also "Andrei-blocked" for Phobos ;)


 Assignment does not really mean much for pure pull requests, 
 in absence of issue tracking - only for rare cases when review 
 is needed by certain qualified person.


 I wish they were rare.  In dmd at least.


Yep I am not very familiar with dmd review process, had Phobos in 
mind.

=====================

I went ahead and have added few labels for Phobos 
(https://github.com/D-Programming-Language/phobos/labels):

"blocked (awaits decision)" == "Andrei-blocked" but named a bit 
more generic just in case
"blocked by dependency" == depends on some other pull request in 
dmd / druntime
"needs work" == pull request author have not responded to review 
comments for some time
"needs review" == pull request in a good shape but needs more 
reviewer input before merging

If it makes sense probably worth making Phobos mail list 
announcement for all maintainers.
Jul 29 2014
next sibling parent reply "safety0ff" <safety0ff.dev gmail.com> writes:
We should consider a label for "revivable" PRs:
PRs which the original submitter is no longer responding but the 
pull can be salvaged if somebody rebases and addresses the 
feedback comments.

I tried using the search "is:unmerged is:pr is:closed" to try and 
find candidates but unfortunately it doesn't seem to work despite 
being documented: 
https://help.github.com/articles/searching-issues#is
Jul 29 2014
parent reply "Daniel Murphy" <yebbliesnospam gmail.com> writes:
"safety0ff"  wrote in message news:hvdsdwbvibtuojxvnbwy forum.dlang.org... 


 We should consider a label for "revivable" PRs:
 PRs which the original submitter is no longer responding but the 
 pull can be salvaged if somebody rebases and addresses the 
 feedback comments.


Tagged as 'needs work' + open + sort by recently updated should do it.
Jul 29 2014
parent reply "safety0ff" <safety0ff.dev gmail.com> writes:
On Tuesday, 29 July 2014 at 14:12:34 UTC, Daniel Murphy wrote:

 Tagged as 'needs work' + open + sort by recently updated should 
 do it.


Ok, this is workable as long as we remove "needs work" labels 
prior to merging pulls.
PRs which are candidates for rebooting might be closed due to 
inactivity.

This isn't too important right now, but I thought I'd toss the 
idea around.
Jul 29 2014
parent reply "Daniel Murphy" <yebbliesnospam gmail.com> writes:
"safety0ff"  wrote in message news:qwdmzdjdwgqgqfrfczxd forum.dlang.org...


 Ok, this is workable as long as we remove "needs work" labels prior to 
 merging pulls.
 PRs which are candidates for rebooting might be closed due to inactivity.

 This isn't too important right now, but I thought I'd toss the idea 
 around.


Yeah.  It would be easy enough to automatically add a 'merged' or 'unmerged' 
tag to all closed pulls in necessary.  I can't see a way to search for pulls 
_without_ a certain label unfortunately.
Jul 29 2014
parent "safety0ff" <safety0ff.dev gmail.com> writes:
On Tuesday, 29 July 2014 at 16:30:27 UTC, Daniel Murphy wrote:

 Yeah.  It would be easy enough to automatically add a 'merged' 
 or 'unmerged' tag to all closed pulls in necessary.  I can't 
 see a way to search for pulls _without_ a certain label 
 unfortunately.


If you look at the link I posted: 
https://help.github.com/articles/searching-issues#is

I believe the merged vs. unmerged functionality is supposed to be 
built in.
Jul 29 2014
prev sibling parent reply "Brad Anderson" <eco gnuk.net> writes:
On Tuesday, 29 July 2014 at 10:42:24 UTC, Dicebot wrote:

 [...]
 I went ahead and have added few labels for Phobos 
 (https://github.com/D-Programming-Language/phobos/labels):

 "blocked (awaits decision)" == "Andrei-blocked" but named a bit 
 more generic just in case
 "blocked by dependency" == depends on some other pull request 
 in dmd / druntime
 "needs work" == pull request author have not responded to 
 review comments for some time
 "needs review" == pull request in a good shape but needs more 
 reviewer input before merging

 If it makes sense probably worth making Phobos mail list 
 announcement for all maintainers.


Good start. The blocked ones are kind of long which is a slight 
problem because Github truncates them in the Pull Request list. 
Maybe rename them to "awaits decision" and "awaits dependency". I 
think "await" implies they are blocked fairly well.
Jul 29 2014
parent "Dicebot" <public dicebot.lv> writes:
On Tuesday, 29 July 2014 at 17:06:55 UTC, Brad Anderson wrote:

 Good start. The blocked ones are kind of long which is a slight 
 problem because Github truncates them in the Pull Request list. 
 Maybe rename them to "awaits decision" and "awaits dependency". 
 I think "await" implies they are blocked fairly well.


Yep, tweaked a bit to fit without truncation.

I am slowly going through least recently updated pull requests 
and adding those labels.
Jul 30 2014