Idea for PRs reviewing workflow

[Ondřej Čertík]

Hi,

I've added few labels related to PR:

PR: needs more work (author's turn)
PR: waiting for Travis to finish

and then we can use them in a query. In particular, to only see PRs that

* are open
* pass all tests
* do not have the "PR: needs more work (author's turn)" label
* sort by recently updated

do:

https://github.com/sympy/sympy/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+status%3Asuccess+sort%3Aupdated-desc+-label%3A%22PR%3A+needs+more+work+%28author%27s+turn%29%22+

Those are all PRs that either can be merged or reviewed. If after the
review the author needs to do more work, just add the label "PR: needs
more work (author's turn)".

Then, when you start your reviewing session, start by looking at the
PRs where it was author's turn, sort by recently updated:

https://github.com/sympy/sympy/issues?q=is%3Aopen+label%3A%22PR%3A+needs+more+work+%28author%27s+turn%29%22+sort%3Aupdated-desc

and remove the label to PRs where the author did the work.

I started using this workflow, it seems it is a lot more manageable.
Let me know what you think. We should refine this and document it.

Ondrej

[Sartaj Singh]

I think it's a good idea. +1 

Just a note. Reviewer should be careful with the 'waiting for Travis to finish' Label. Sometimes it's just a timeout Error. So, errored build should be kept in mind while assigning the labels. 

--
You received this message because you are subscribed to the Google Groups "sympy" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.
To post to this group, send email to sy...@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.
To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/CADDwiVAMnWPw81ow04n9BmAArMYfb1_9EAx5WAFWNLffn1xQAw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--

Regards

Sartaj Singh

Mathematics and Computing,

Indian Institute of Technology,

Varanasi - 221 005 INDIA

E-mail: singhs...@gmail.com, sartaj.si...@itbhu.ac.in

[Ondřej Čertík]

On Tue, Jun 9, 2015 at 2:31 PM, Sartaj Singh <singhs...@gmail.com> wrote:
> I think it's a good idea. +1
>
> Just a note. Reviewer should be careful with the 'waiting for Travis to
> finish' Label. Sometimes it's just a timeout Error. So, errored build should
> be kept in mind while assigning the labels.

I am pretty sure that builds that time out eventually turn into the
red or gray cross, don't they?

So if we only assign the "Travis to finish" label to yellow statuses,
that should be robust.

Ondrej

> https://groups.google.com/d/msgid/sympy/CAC%2BH8-FpuzY_-i4Jn9ON6hm9gj52eezoTVsNE3SzELASDi1WLA%40mail.gmail.com.

[Sudhanshu Mishra]

I am pretty sure that builds that time out eventually turn into the

red or gray cross, don't they?

​They show "Errored" instead of "Failed".​

Sudhanshu Mishra

To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/CADDwiVD8yN5TGSoGB%2BjKTHUK9NqiE-g%2B%2BEnqcfNh40DU6KnaJg%40mail.gmail.com.

[Aaron Meurer]

FYI you can sort pull requests by Travis CI status https://github.com/blog/2014-filter-pull-requests-by-status.

Aaron Meurer

To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/CAFW_KCSfSgptWanVt7eX4rjRX9bjMDBtoC3Ab4R2kwELkUyVWQ%40mail.gmail.com.

[Joachim Durchholz]

> I've added few labels related to PR:
>
> PR: needs more work (author's turn)
> PR: waiting for Travis to finish
>
> and then we can use them in a query.

Only contributors (i.e. people with repository write access) can set or
remove these labels. I.e. most authors can't remove "needs more work"
when they think they're done, and they can't set a "please review" label.

I am currently working on a WSGI script that extracts label-setting
commands from PRs, e.g. [+WIP] to add the WIP label and [-WIP] to remove it.
It's not done yet (and will take a while until it is), but I can set up
a GitHub project to share the code if anybody is interested in speeding
this up.

> Those are all PRs that either can be merged or reviewed. If after the
> review the author needs to do more work, just add the label "PR: needs
> more work (author's turn)".

E.g. I'd be unable to add that label, just like anybody else who does
reviews but does not have write access.

[Jason Moore]

Ondrej and I thought that we should ask Github to enable PR authors' ability to modify labels on their own PRs. We should all put in a request for that.

Jason
moorepants.info
+01 530-601-9791

--
You received this message because you are subscribed to the Google Groups "sympy" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.
To post to this group, send email to sy...@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.

To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/55775922.3000005%40durchholz.org.

[Joachim Durchholz]

Am 09.06.2015 um 23:39 schrieb Jason Moore:
> Ondrej and I thought that we should ask Github to enable PR authors'
> ability to modify labels on their own PRs. We should all put in a request
> for that.

Labels such as "ready to merge if Travis is okay with it" should not be
settable by the PR author, so I doubt GH will implement that.

[Jason Moore]

I don't think Github cares how we manage our projects. This is only about enabling us to manage it how we want/need. If they have a checkbox in the repo's settings that allows/disallows PR authors to modify labels then we can decide how much freedom we want to give an author.

Jason
moorepants.info
+01 530-601-9791

--
You received this message because you are subscribed to the Google Groups "sympy" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.
To post to this group, send email to sy...@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.

To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/55775EDC.9010600%40durchholz.org.

[Ondřej Čertík]

On Tue, Jun 9, 2015 at 3:14 PM, Aaron Meurer <asme...@gmail.com> wrote:

FYI you can sort pull requests by Travis CI status https://github.com/blog/2014-filter-pull-requests-by-status.

 

That's where I got the idea to filter by passing tests. Coming to think about it, the label "PR: waiting for Travis to finish" is probably not necessary. One can quickly filter by status:pending to get those.


Aaron Meurer

On Tue, Jun 9, 2015 at 3:44 PM, Sudhanshu Mishra <mrs...@gmail.com> wrote:

I am pretty sure that builds that time out eventually turn into the

red or gray cross, don't they?

​They show "Errored" instead of "Failed".​

I filter by "tests pass", more specifically "status:success".

You are right that one should then look into other PRs that do not pass for whatever reason and help people out. The part where we help people doesn't change.

The above workflow is for making sure that once the author makes sure that tests pass, we move forward quickly, i.e. clearly mark that it is his/her turn to do something "PR: needs more work (author's turn)", or I need to do something, i.e. review and either add "PR: needs more work (author's turn)" or merge.

Also notice that I sort by last update. Again, this all ensures that people who submit PRs that are high quality can quickly pass review and get merged, in other word, that they quickly propagate through our huge PR queue.

I am thinking of also adding "PR: in review" for huge PRs that require some time and multiple people to review, or require some kind of a decision to be made or a solution to be found. This label should be added for PRs that need review as well. Perhaps we can suggest a better name for label.

That way, each PR is in one (and only one) of the following stages:

* tests are in progress (the yellow status)
* labelled "PR: needs more work (author's turn)"

* labelled "PR: in review"

* None of the above -- in this case it is a new PR and somebody needs to do the review and add a label

Then we can easily go through our PR queue, label things and no need to worry anymore about a huge queue. In fact, we can keep PRs in there, because nothing will get lost, and we can easily filter using labels, so there is no issue and we do not need to close inactive PRs and open an issue for it.

As a reviewer, here is the workflow:

* go over all recent "PR: needs more work (author's turn)", and if the author did the work, flip the label (hopefully this can be automated with some script --- i.e. determining when the label was added and if any activity happened since then, and if so, the script can remove the label)

* review all unlabeled PR

* review other PRs, either with "PR: in review" or with tests still running

Ondrej

[Ondřej Čertík]

How about calling these:

PR: author's turn
PR: sympy's turn

[Ondřej Čertík]

> How about calling these:
>
> PR: author's turn
> PR: sympy's turn

Another idea is:

PR: author's turn
PR: author is done


Let's find some good names for the labels.

[Joachim Durchholz]

Am 09.06.2015 um 23:55 schrieb Jason Moore:
> I don't think Github cares how we manage our projects.

Sort of. There is a "GitHub workflow", and not all git workflows are
easily mappable to it (git-flow, for example, won't - not easily anyway).

Though I agree that they probably don't care about labels.

> This is only about
> enabling us to manage it how we want/need. If they have a checkbox in the
> repo's settings that allows/disallows PR authors to modify labels then we
> can decide how much freedom we want to give an author.

It would need to be settable on a per-label basis.
We want some labels to be settable only by people who can merge, and
some labels settable by everyone. Similarly for resetting.

Examples:

We don't want "ready to merge if Travis is okay" to be settable by
everybody, we want to reserve that for those who can actually do a merge.
However, we want to allow the PR author and any reviewer to remove it.

For the "needs more work" label, reviewers and the PR author should be
able to set and to remove it.

[Joachim Durchholz]

Am 10.06.2015 um 00:22 schrieb Ondřej Čertík:
>> That way, each PR is in one (and only one) of the following stages:
>>
>> * tests are in progress (the yellow status)
>> * labelled "PR: needs more work (author's turn)"
>> * labelled "PR: in review"
>
> How about calling these:
>
> PR: author's turn
> PR: sympy's turn

+1. "Needs more work" tends to look like criticism, and not everybody is
happy to receive criticism.

[Ondřej Čertík]

First of all, we can trust the PR authors with stuff like this. The
one how does the actual merge must review the PR anyway (since he is
ultimately responsible), and see who set what labels. So if the PR
author is trying to deceive us, we would found out anyway.

Second, in my workflow, I propose only two labels (author's turn /
sympy's turn). You can post a comment that you are ok with merging
this if tests pass.

Ondrej

>
> For the "needs more work" label, reviewers and the PR author should be able
> to set and to remove it.
>

> --
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to sympy+un...@googlegroups.com.
> To post to this group, send email to sy...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sympy.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/sympy/557767DB.1000903%40durchholz.org.

[Ondřej Čertík]

I agree. Sometimes it doesn't really need more work, just a comment /
clarification or perhaps only a final approval/permission/blessing
from the author to merge this.

It's really about who's turn it is. So that both reviewers as well as
authors can see if they need to do anything.

Ondrej

[Joachim Durchholz]

Am 10.06.2015 um 00:37 schrieb Ondřej Čertík:
> It's really about who's turn it is. So that both reviewers as well as
> authors can see if they need to do anything.

Another potential approach is "what needs to be done next"?
The label set that came out of this thought was this:


Needs Evaluation: Projekt must find out what needs to be done, more for
Issues than for PRs but occasionally for PRs as well.

Needs Work: It's clear what to do, for PRs the PR author needs to do
something, for Issues somebody needs to pick this up and start working
on it.

Needs Review: The PR author thinks the PR should be reviewed.

Needs More Review: After the first review passed, either reviewer or PR
author thinks another review would be beneficial.

Needs Merge: PR author and reviewer are happy with it but couldn't merge
(either because the reviewer cannot merge, or because Travis hasn't
finished yet).

Needs Travis Restart: Travis hung and needs to be restarted, but the
person who noticed that doesn't have permission to do so.


With that, the "needs more work" label is back, but it lost its
potentially harsh overtone because now everybody is being told that they
need to do stuff.

[Joachim Durchholz]

Am 10.06.2015 um 00:34 schrieb Ondřej Čertík:
>> We don't want "ready to merge if Travis is okay" to be settable by
>> everybody, we want to reserve that for those who can actually do a merge.
>> However, we want to allow the PR author and any reviewer to remove it.
>
> First of all, we can trust the PR authors with stuff like this.

I'm more worried about mistakes than about intentional deception.
(Oh and then there's trolling, but trolls get banned eventually.)

> The
> one how does the actual merge must review the PR anyway (since he is
> ultimately responsible), and see who set what labels. So if the PR
> author is trying to deceive us, we would found out anyway.

Having to check that will be additional work.
Though... probably unavoidable, the author could have added more commits
before Travis finishes, so "okay to merge after Travis success" is
probably not that useful, I guess such status labels would need to be
removed with every update to the PR.

> Second, in my workflow, I propose only two labels (author's turn /
> sympy's turn). You can post a comment that you are ok with merging
> this if tests pass.

You're right - I thought each should be settable only by one party and
resettable by the other, but after some more thinking, I see that both
PR author and SymPy PR gatekeepers need to set and reset both labels.

Still, I do see use cases for per-label configuration.

[Joachim Durchholz]

Am 09.06.2015 um 23:39 schrieb Jason Moore:

> Ondrej and I thought that we should ask Github to enable PR authors'
> ability to modify labels on their own PRs. We should all put in a request
> for that.

Once you put in a request, please post the link so we can add comments.

Thanks :-)

[Jason Moore]

https://github.com/contact

Jason
moorepants.info
+01 530-601-9791

--
You received this message because you are subscribed to the Google Groups "sympy" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.
To post to this group, send email to sy...@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.

To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/5577F7C2.3010701%40durchholz.org.