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

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

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.

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.

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

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.

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

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

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.

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