Hi Stéfan,

I think figuring this out is certainly a high priority when it comes to being contributor-friendly.

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?

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. And the current PRs on manual segmentation and nD rescaling 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.

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.

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.

**: by minor, I mean not something like "class Filter: …” "class Gaussian(Filter): …” =P

So, to summarise, my proposal when dealing with non-core contributors:

- avoid massive API discussions. Settle on the simplest solution and add a to-do. If it’s new functionality, put it in skimage.future.
- avoid all kinds of style nitpicking — it’s faster and easier to fix these things ourselves. I suggest even doing optimistic merging.
- don’t ask users to rebase unless it’s clear that they are git experts. Follow github’s advice and merge on the command line.

Comments are very welcome!

Juan.

On 7 Apr 2017, 10:32 AM +1000, Stefan van der Walt <stefanv@berkeley.edu>, wrote:
Hi everyone, and especially devs

This afternoon, Nelle Varoquaux gave a short presentation at BIDS on
optimistic merging, a workflow suggested by the late Pieter Hintjens.
In optimistic merging, all PRs are merged into a project, and fixes
happen via additional patches or reverts. While, in my mind, this is
not a viable model for scikit-image, it made me think of our PR review
process.

A failure mode I observe often is the following:

- user submits patch
- review happens, often either a) nitpicking on minor things or b)
asking for technically difficult tasks to be performed (rebasing, can be
tricky)

This introduces several problems, including discouraging new
contributors, and wasting a lot of time (many PRs are left hanging for
months, while waiting for a one-line fix from the original author).

I'd like to request the following:

- Ask contributors to open their branches for collaboration (this is on
by default already, probably).
- While reviewing a PR, pull down the branch, look at the diff, and fix
any trivial errors you see. Push the results back up or, if
collaboration is switched off, make a PR against theirs.
- Keep review comments to bigger picture changes, and avoid getting
bogged down in trivial fixes and stylistic changes.

Let's keep it collaborative and friendly, grow our team of contributors,
and get going on 0.14 :)

Thanks!
Stéfan



P.S. A question for the git experts:

I've been looking for ways of making the above process easier. I found
that I can easily download pull requests:

- In my ~/.gitconfig, I add the following alias (replace 'upstream'
with whatever you normally call the scikit-image main repository):

[alias]
pullpr = "!f() { git fetch upstream pull/$1/head:pr$1 ;}; f"

I can now grab a PR with:

git pullpr 2447

and it will appear in the branch pr2447.

Unfortunately, I don't have a similarly easy way of pushing back, other
than adding the contributor's remote, and pushing back to the PR branch.
Any ideas?

_______________________________________________
scikit-image mailing list
scikit-image@python.org
https://mail.python.org/mailman/listinfo/scikit-image