[hibernate-dev] Retrospective on "Pull Requests": a waste of time?

[Sanne Grinovero]

On 10 July 2013 13:00, Emmanuel Bernard <emmanuel at hibernate.org> wrote:
> On Wed 2013-07-10 11:09, Hardy Ferentschik wrote:
>>
>> >> What is the definition of not fully on topic. I would not suggest a change in
>> >> class X for a pull request where only class Y and Z are affected. However,
>> >> if class X is touched and I see a potential improvement I think it can be considered
>> >> being part of the topic. Boy Scout rule number one:" Always leave the campground
>> >> cleaner than you found it."  I truly believe in this one, but of course sometimes a
>> >> potential improvement would have too big of a ripple effect to be pursued.
>> >
>> > I think that's the crux of the disagreement.
>>
>> To a degree yes.
>>
>> > Disclaimer, it depends but if the cumulated changes take 5 mins or 3 hours things vary.
>>
>> There is for sure a limit where we are not talking about keeping the campground clean anymore.
>>
>> > Breaking the flow of a small or medium sized PR can be problematic IMO.
>>
>> What is "breaking the flow" of a pull request? Are you saying that just because the reviewer
>> of the pull request discovered potential points of improvement or suggests other fixes, it
>> breaks the work flow of submitter of the pull request, because he did not get an immediate merge?
>>
>> What's the point of reviewing if we are not able to discuss potential improvements. Do you want
>> feedback or do you want someone to press the merge button?
>>
>> If your work really depends on this one particular pull request being merged I argue that you should not
>> have submitted it and do a combined pull request. Or as mentioned before just keep on working on
>> your local branch. You have plenty of choices to proceed and there is no need to be blocked on a pull request.
>>
>> The only problematic case I see is, if there is an immediate release required, because an external consumer (e.g Infinispan requiring an updated
>> version of Search with a specific bug fix). However, that is not what we have been talking about here.
>>
>> The moment you submit a pull request you are saying that you want this work to be reviewed and merged
>> to master. At this point you have to be willing to deal with the feedback. If not, the whole procedure becomes pointless.
>
> Let's take an example. Someone works on say spatial search and that by
> side effect DocumentBuilder is changed and that the reviewer comments
> that some part of DocumentBuilder logic needs to be rewritten or that
> some method / classes related to DocumentBuilder need to be renamed. The
> logic at bay is used by the spatial change but has been there for a
> while. And that logic everyone agrees has room for improvement.
>
> Option A, you rework that logic as part of the spatial feature PR. I am
> claiming that this is breaking the flow of getting spatial out the doors
> unnecessarily.
>
> Option B, the reviewer comment is converted in a JIRA that can be
> addressed as soon as the spatial query is pushed to master.
>
> Option C, the reviewer must shut up and only comment on the core PR
> feature.
>
> I am in favor of B, I do ask for feedback on the spatial feature and
> having to deal with this not quite related improvement is moving my
> focus and short memory away from the spatial problem.
> Once I'm done, I can work on the reported issue of course.

+1000