new github features

Development matplotlib-devel
1

Folks,

Have people had a chance to look at the new review tools on github? Do we
want to work these into our workflow? It looks like we can set it so that
PRs can only be merged via the web UI with at least one approve review and
no 'needs work' reviews if we want. My initial reaction is we should try
to use the review tools, but hold on on the engineering control on merges
until we have a better sense of what that would look like.

Has anyone looked at the 'projects' feature yet?

A suggestion from Nelle Varoquaux is to follow skimage/sklearn and on
review add a [mrg+N] to the title to help other devs find PRs that are
almost ready to merge, but need another set of eyes.

Tom
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20160920/7dc4884c/attachment.html>

2

Hi all,

One data point, it seem that some users/dev (at least on Jupyter side)
are complaining that they don't get the "grouped-review-comments" by
emails. So are waiting for feedback they never get until they check
the PR.

Still haven't figured out when this happens.

I'm curious if the "Approved" review get reset after a new commit is
pushed, and If so and the N reviewer is enforced, it can be annoying
if someone approved with a small condition, like typo fixed or
something similar.

···

--
M

On Mon, Sep 19, 2016 at 7:24 PM, Thomas Caswell <tcaswell at gmail.com> wrote:

Folks,

Have people had a chance to look at the new review tools on github? Do we
want to work these into our workflow? It looks like we can set it so that
PRs can only be merged via the web UI with at least one approve review and
no 'needs work' reviews if we want. My initial reaction is we should try
to use the review tools, but hold on on the engineering control on merges
until we have a better sense of what that would look like.

Has anyone looked at the 'projects' feature yet?

A suggestion from Nelle Varoquaux is to follow skimage/sklearn and on review
add a [mrg+N] to the title to help other devs find PRs that are almost ready
to merge, but need another set of eyes.

Tom

_______________________________________________
Matplotlib-devel mailing list
Matplotlib-devel at python.org
Matplotlib-devel Info Page

3

Folks,

Have people had a chance to look at the new review tools on github? Do
we want to work these into our workflow? It looks like we can set it so
that PRs can only be merged via the web UI with at least one approve
review and no 'needs work' reviews if we want. My initial reaction is
we should try to use the review tools, but hold on on the engineering
control on merges until we have a better sense of what that would look like.

I'm not a fan of this new mechanism. One of the problems is that
sometimes one doesn't know at the outset whether one will be doing a
thorough review, or just starting to look and make a few comments. I
don't much like the automated comments it generates based on the review
checkbutton, either. Making this mechanism mandatory would be quite an
irritant for little or no benefit, I think.

Some automated operations are good. For example, if a "closes #1234" in
an obvious location in a PR causes #1234 to be closed when the PR is
merged, that's good. Unfortunately, I haven't figured out how this
works--it seems like sometimes it does, sometimes it doesn't.
Presumably I just don't know the rule github is using. So here, a
better design would be a box in the PR specifically for linking in
"closes" issue numbers.

Thumbs-up emoji counts are not helpful; I need to look to see whose
thumb it is, and then I'm still left wondering exactly what was being
approved. And was it a thoughtful approval, or just a twitch?

So overall, with my usual curmudgeon's hat on, covering my small brain,
I favor keeping the automation simple, obvious, and fairly minimal. For
everything else, let's use normal human communications, not bots.

Has anyone looked at the 'projects' feature yet?

Haven't looked. Should I?

A suggestion from Nelle Varoquaux is to follow skimage/sklearn and on
review add a [mrg+N] to the title to help other devs find PRs that are
almost ready to merge, but need another set of eyes.

Aha! So that's what those are! The "N" is the number of eye-sets needed?
I don't object to having a few conventions like that. If it ends up
helping us cut down the open PR and issue counts, that would be great.
Conventions are better than bots.

Eric

···

On 2016/09/19 4:24 PM, Thomas Caswell wrote:

4

I think the projects board (it looks like a trello type thing) could be a
good way to organize MEPs and/or new feature requests with a lot of moving
pieces (like the categorical work...)

···

On Mon, Sep 19, 2016 at 10:24 PM, Thomas Caswell <tcaswell at gmail.com> wrote:

Folks,

Have people had a chance to look at the new review tools on github? Do we
want to work these into our workflow? It looks like we can set it so that
PRs can only be merged via the web UI with at least one approve review and
no 'needs work' reviews if we want. My initial reaction is we should try
to use the review tools, but hold on on the engineering control on merges
until we have a better sense of what that would look like.

Has anyone looked at the 'projects' feature yet?

A suggestion from Nelle Varoquaux is to follow skimage/sklearn and on
review add a [mrg+N] to the title to help other devs find PRs that are
almost ready to merge, but need another set of eyes.

Tom

_______________________________________________
Matplotlib-devel mailing list
Matplotlib-devel at python.org
Matplotlib-devel Info Page

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20160920/ac92e40d/attachment.html&gt;

5

Hi Eric,

Some automated operations are good. For example, if a "closes #1234" in an obvious location in a PR causes #1234 to be closed when the PR is merged, that's good. Unfortunately, I haven't figured out how this works--it seems like sometimes it does, sometimes it doesn't. Presumably I just don't know the rule github is using. So here, a better design would be a box in the PR specifically for linking in "closes" issue numbers.

I can inform you on that.

It has to be in one of the _commit message_, once the commit is in
master the PR get closed.

It is confusing as when there is a single commit, GitHub make the the
default for the pull-request description,
but being in the commit allows you to close issues by pushing directly
on master (for project that do it).

See Linking a pull request to an issue - GitHub Docs

Cheers,

···

--
M

On Mon, Sep 19, 2016 at 8:00 PM, Eric Firing <efiring at hawaii.edu> wrote:

On 2016/09/19 4:24 PM, Thomas Caswell wrote:

Folks,

Have people had a chance to look at the new review tools on github? Do
we want to work these into our workflow? It looks like we can set it so
that PRs can only be merged via the web UI with at least one approve
review and no 'needs work' reviews if we want. My initial reaction is
we should try to use the review tools, but hold on on the engineering
control on merges until we have a better sense of what that would look
like.

I'm not a fan of this new mechanism. One of the problems is that sometimes
one doesn't know at the outset whether one will be doing a thorough review,
or just starting to look and make a few comments. I don't much like the
automated comments it generates based on the review checkbutton, either.
Making this mechanism mandatory would be quite an irritant for little or no
benefit, I think.

Some automated operations are good. For example, if a "closes #1234" in an
obvious location in a PR causes #1234 to be closed when the PR is merged,
that's good. Unfortunately, I haven't figured out how this works--it seems
like sometimes it does, sometimes it doesn't. Presumably I just don't know
the rule github is using. So here, a better design would be a box in the PR
specifically for linking in "closes" issue numbers.

Thumbs-up emoji counts are not helpful; I need to look to see whose thumb it
is, and then I'm still left wondering exactly what was being approved. And
was it a thoughtful approval, or just a twitch?

So overall, with my usual curmudgeon's hat on, covering my small brain, I
favor keeping the automation simple, obvious, and fairly minimal. For
everything else, let's use normal human communications, not bots.

Has anyone looked at the 'projects' feature yet?

Haven't looked. Should I?

A suggestion from Nelle Varoquaux is to follow skimage/sklearn and on
review add a [mrg+N] to the title to help other devs find PRs that are
almost ready to merge, but need another set of eyes.

Aha! So that's what those are! The "N" is the number of eye-sets needed?
I don't object to having a few conventions like that. If it ends up helping
us cut down the open PR and issue counts, that would be great.
Conventions are better than bots.

Eric

_______________________________________________
Matplotlib-devel mailing list
Matplotlib-devel at python.org
Matplotlib-devel Info Page

6

Hi,

In general, given the griping I've seen over time for how hard it is to do
code review on GitHub Pull Requests, I think we (the open source community)
owe it to GitHub to try out the new feature they rolled out to address and
give them feedback--that's the only way the feature can ever meet our needs.

For our (matplotlib) case, I think we get a few good gains:
- line comments are batched so we can cut down on the number of
notifications
- Your work is saved so you can come back later and finish--and adjust when
you see something later that invalidates your comment *before* commenting
for all to see
- Reviews are atomic items, so you can get an idea of what a specific
reviewer thinks (and not lose pieces)
- By having the state of Approved/Needs Changes actually tracked, there's
no need to read through the entire PR to see what objections are left.
(Granted, it does mean people have to come back and approve it once the
changes are made)

Seems like a lot to gain for little to no extra effort--having used it, I
certainly fail to see any problem. The matter of turning on required
reviews for protected branches is a whole other matter, though I will say
you can always let admins override.

Ryan

···

On Mon, Sep 19, 2016 at 8:24 PM, Thomas Caswell <tcaswell at gmail.com> wrote:

Folks,

Have people had a chance to look at the new review tools on github? Do we
want to work these into our workflow? It looks like we can set it so that
PRs can only be merged via the web UI with at least one approve review and
no 'needs work' reviews if we want. My initial reaction is we should try
to use the review tools, but hold on on the engineering control on merges
until we have a better sense of what that would look like.

Has anyone looked at the 'projects' feature yet?

A suggestion from Nelle Varoquaux is to follow skimage/sklearn and on
review add a [mrg+N] to the title to help other devs find PRs that are
almost ready to merge, but need another set of eyes.

Tom

_______________________________________________
Matplotlib-devel mailing list
Matplotlib-devel at python.org
Matplotlib-devel Info Page

--
Ryan May
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20160921/265347a3/attachment.html&gt;

7

Matthias,

Regarding the emails, could this be due to notification settings? I notice
that PR reviews are a separate item you have to check in the notification
center.

Ryan

···

On Mon, Sep 19, 2016 at 8:32 PM, Matthias Bussonnier < bussonniermatthias at gmail.com> wrote:

Hi all,

One data point, it seem that some users/dev (at least on Jupyter side)
are complaining that they don't get the "grouped-review-comments" by
emails. So are waiting for feedback they never get until they check
the PR.

Still haven't figured out when this happens.

I'm curious if the "Approved" review get reset after a new commit is
pushed, and If so and the N reviewer is enforced, it can be annoying
if someone approved with a small condition, like typo fixed or
something similar.
--
M

On Mon, Sep 19, 2016 at 7:24 PM, Thomas Caswell <tcaswell at gmail.com> > wrote:
> Folks,
>
> Have people had a chance to look at the new review tools on github? Do
we
> want to work these into our workflow? It looks like we can set it so
that
> PRs can only be merged via the web UI with at least one approve review
and
> no 'needs work' reviews if we want. My initial reaction is we should
try
> to use the review tools, but hold on on the engineering control on merges
> until we have a better sense of what that would look like.
>
> Has anyone looked at the 'projects' feature yet?
>
> A suggestion from Nelle Varoquaux is to follow skimage/sklearn and on
review
> add a [mrg+N] to the title to help other devs find PRs that are almost
ready
> to merge, but need another set of eyes.
>
> Tom
>
> _______________________________________________
> Matplotlib-devel mailing list
> Matplotlib-devel at python.org
> Matplotlib-devel Info Page
>
_______________________________________________
Matplotlib-devel mailing list
Matplotlib-devel at python.org
Matplotlib-devel Info Page

--
Ryan May
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/matplotlib-devel/attachments/20160921/9bf3b89c/attachment.html&gt;