[Qgis-psc] Working on pull requests

[Matthias Kuhn]

Hi

On 09/01/2015 06:49 AM, Tim Sutton wrote:
> Hi
>
>> I'm not sure this
>> is really needed. There's a lot of PRs, but most of them are either
>> work in progress or waiting on changes from the original submitter
>> after developer feedback. Matthias already does a good job keeping the
>> list maintained and pinging various people for attention (so that said
>> - maybe he'd like funding to keep doing this!).
>
> Hmm good feedback - maybe we should chat to Matthias and see what is
> needed from his point of view…thanks!

I think a formalized process would be good to handle PR's but
unfortunately I don't have the perfect approach ready. Maybe somebody
can provide a good concept from other projects.

Some general thoughts about it:

Pull requests are a mix of ready-to-be-merged (or almost) and prototype
pull requests.
The first ones are (from the authors perspective) ok to merge. The
second type is more asking for general feedback. Type 1  PR's are often
opened by external parties (possible future coders), type 2 often by
core committers (used as show-room for your local R&D). Therefore IMO
the first ones deserve more attention. However, the signal to noise
ratio in the PR queue makes it hard to separate the two, what scares
reviewers off. And then there is a third kind of old pull requests which
are hard to merge because the code has drifted away too far.

I once introduced a label "prototype" so core committers could tag their
PR's as such and when looking through the queue one could filter them.
The tag has been removed. The use of the tag has not been
well-communicated at that time, this may have been the reason. I think
it would be worth thinking about if and how tags can be used to improve
the process.

Closing a pull request (instead of merging) involves disapointing
somebody and dismissing possible improvements. I assume that sometimes
reviewers are scared to dismiss pull requests. If they do not get merged
anyway, they end up as zombies in the PR queue and add more noise to the
signal. I think a checklist of "what does a PR need" would be good to
point to and say "hey, sorry, but see, that's why it's not possible to
merge it".

Some time ago somebody (I think Marco H.) used to assign reviewers to
pull requests. I think that this helped to motivate people to perform
review tasks. However, if tasks sticked to somebody with no time or
unwilling to review, these PR's would get forgotten. I think by
repeatedly pinging the responsible person or in case of unavailability
reassigning to somebody else would be good.

Bottomline:
It would be good to have a prompt reaction for type 1 pull requests. It
motivates people and early merging gives time to test before the release
(or even better before the freeze). Therefore it would be great to keep
the review queue of type 1 pull requests short. This can be done by 1.
quickly merging easy fixes, 2. merging after asking the author for
comments, explanations, improvements or 3. closing / dismissing the PR.

What I think is important is to find a good way to assign the
responsible dev for review, identify common causes which hold reviewers
back from proceeding and developing tools and measures which help to
counteract (like checklists and questions which may be asked like "can
you provide a test", "can you explain a use-case"...).

Sorry for the long text but I hope this helps to move forward,

Regards,
Matthias

> Regards
>
> Tim
>
>>
>> Nyall
>>
>>
>>
>>
>>>
>>>
>>> _______________________________________________
>>> Qgis-psc mailing list
>>> Qgis-psc at lists.osgeo.org <mailto:Qgis-psc at lists.osgeo.org>
>>> http://lists.osgeo.org/mailman/listinfo/qgis-psc
>>>
>>>
>>>
>>>
>>>
>>> Tim Sutton
>>> QGIS Project Steering Committee Member
>>> tim at qgis.org
>>>
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Qgis-psc mailing list
>>> Qgis-psc at lists.osgeo.org
>>> http://lists.osgeo.org/mailman/listinfo/qgis-psc
>> _______________________________________________
>> Qgis-psc mailing list
>> Qgis-psc at lists.osgeo.org <mailto:Qgis-psc at lists.osgeo.org>
>> http://lists.osgeo.org/mailman/listinfo/qgis-psc
>
>
>
>
> Tim Sutton
> QGIS Project Steering Committee Member
> tim at qgis.org <mailto:tim at qgis.org>
>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/qgis-psc/attachments/20150907/a565e3ee/attachment.html>