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 <stef...@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
_______________________________________________
scikit-image mailing list
scikit-image@python.org
https://mail.python.org/mailman/listinfo/scikit-image
