[scikit-image] How to move pull requests forward (devs, please read)

[Stefan van der Walt]

Hi Juan



On Wed, Apr 12, 2017, at 23:39, Juan Nunez-Iglesias wrote:

> One thing I don’t know what to do is figure out whether a contributor
> has turned on “collaboration”. What am I missing? I can see when
> *I’ve* turned it on, but for other pull requests I can’t get any info.
> Is the only way to know to try to push?


It's enabled by default (still, odd that they do not show it).



> A related point of failure that I’ve see is where we have an API
> discussion on the PR, and the core members have a back and forth over
> it, with confusion about the final outcome for the PR author. I’m
> still traumatised by this comment[1]. And the current PRs on manual
> segmentation[2] and nD rescaling[3] risk the same fate. Stéfan and I
> briefly discussed this off-list and we came up with the following
> solution (which we now request comments on):
> 

> - new functions of dubious API go to skimage.future. That’s what the
>   package was created for. *However*, RAG has languished there for
>   several releases, so this is not a long-term solution. So, in
>   addition:
> - make a note in the TODO.txt for two releases down (would be v0.15
>   currently), forcing an API discussion and a move out of
>   skimage.future for that release.
> - For existing functions that require an API change, merge a “good
>   enough” API, raise an issue on GitHub for the API discussion, and
>   tag it with the next release.


The one concern I have is that we're going to run into the situation
where each release is blocked by a bunch of hard-to-make API decisions.
So perhaps raise an issue for API review of that function, tagged with
next release?  That way, we can start gathering feedback, and even close
before the next release comes along.


> For rebasing, if there is nothing left to do in the PR but the rebase,
> I suggest that we merge the PR manually in the command line.


Why don't we make the following simple rule:



. Never request rebases---instruct contributor to merge, or do
that for them
1. Always squash merges



This is easy for the contributor, and does not pollute the master
branch history.


I know this goes against the original decision to retain all patches in
a PR, but the problem of engaging contributors is much larger than my
somewhat anal preference for well groomed patch-sets.  If a squashed PR
generates an overly large patch, it probably should be broken up anyway.


> Incidentally, regarding optimistic merging: I do think there is a
> place for it in scikit-image, namely, for any PRs that pass tests and
> coverage checks, but need stylistic updates. Frankly, it is faster to
> do these oneself than to write them in the PR, *and* they inevitably
> feel petty to both the reviewer and the contributor. Therefore, I
> might suggest that we do perform optimistic merging when there are
> minor** style issues, and then fix those issues ourselves. However, if
> you merge in this situation, you *must* submit the fix immediately.
> Otherwise it’s certain to be lost.


Whenever you have a queuing system that grows more quickly than you can
consume (e.g., the number of emails in your INBOX), you have to make an
adjustment.  And I think this is a very reasonable compromise.


Stéfan


Links:

  1. https://github.com/scikit-image/scikit-image/pull/953#issuecomment-43598826
  2. https://github.com/scikit-image/scikit-image/pull/2584
  3. https://github.com/scikit-image/scikit-image/pull/1522
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/scikit-image/attachments/20170413/bd0f2d24/attachment-0001.html>