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

[Hardy Ferentschik]

Comments inline:


On 9 Jan 2013, at 5:56 PM, Sanne Grinovero <sanne at hibernate.org> wrote:

> I also don't think we can nail down precise "rules" .. I will just ask
> to use common sense and try hard to merge pulls quickly.

what do you want, a review or an automatic merge? Some pull requests can
be merged quickly, some take more time. I find a lot of the discussions very fruitful.

> To add some specific directions: I noticed frequently some good points
> are made on a PR which are not strictly a blocker to the pull itself.
> Of course if the tests fail or if it introduces other blockers which
> prevent other developers to continue on their work, it should not be
> merged.. but in most other cases we should try to ask for "follow up"
> improvements rather than "fix now".

Speaking out of my experience, this would just create a lot of follow up Jira issues,
many of which will be in the system for ever. Aren't there always more important things
to do? IMO, if code gets touched and we agree that there are potential improvements 
we can apply, why not taking the time to do this? 

I also fail to see, how these pull request stop you from continue working. You have multiple 
choices. Take your Infinispan upgrade for example. You could have chosen to create a 
a combined pull request for the upgrade and whatever depends on it. You could
have also locally based your follow up work on your branch with the Infinispan upgrade.
At some point the upgrade would make it to master (as it did) and you could
have rebased onto that. 

> If we can create follow-up improvements, we can easily prioritize
> them. Decoupling the priority of further improvements from the main
> goal of the pull request is important

And then we can just ignore these follow ups, because they are not important, right? ;-)

> to make sure that we're spending
> time on the highest value instead of entertaining ourselves with
> design puzzlers..

It would help if you would give some concrete examples. I cannot remember a single 
discussion which was held for pure "entertainment"

> I know the feeling, we all are scientists and enjoy
> some nitty gritty polishing but often there's no real gain from it,

There is a huge gain and that's in maintainability and the ability to understand the code.
You keep talking about the time loss for not being able to build new functionality. 

What about the time loss  of trying to understand existing entangled  code in order to
implement some new functionality?

> not even long term readability as I'd rather hope our code to evolve
> faster.. we're not polishing a statue of immutable stone.

Ok, let's skip coding styles then as well. They only slow me down and readability 
is not an issue, because the code changes anyways.

> I suspect that could have some good effects:
> - no push-force on the same pull hiding previous discussions

GitHub is quite good now in showing these outdated comments. 

> - no long lists of dormant pull requests

I think the highest open pull request count on Search I've seen is 4 or 5.
That is reasonable imo, especially since it usually gets down again very quickly.
The only exception being pull #405 which is sitting in the pull request queue for
a long time now. However, this pull request falls into the preview pull request
category you are mentioning below, since it is not even complete. I vote we
close this one.

>  I think this would also encourage people to be quick in merging if
> there is *any* open pull request.

I think we are quick. Also I suggest we should be quick in reviewing, not in merging.
A fine difference.

> # Avoid pull requests for previews

+1 (although we would need to define what a preview pull request actually is)

> # Avoid showing off scanner capabilities :-)
> I've been very guilty myself on this one; I remember occasionally to
> have attacked a PR with a sense of "let's see how many nitty gritty
> details I can find", then effectively pointing out many typos and
> small inaccuracies but possibly failing to evaluate the grand plan.

There is for a balance to be struck, but I don't like the idea of ignoring things.
And sometimes the grand plan is great, but the execution sucks.

> Let's fight our love for perfection and bring home some *useful* stuff
> ASAP. (Hint: renaming internal classes is not in this category)

Ok, I guess now we are talking about HSEARCH-1355 and HSEARCH-1356.
Taking the former for example. Here we renamed IndexBinder to IndexBinding. 
IMO there is a huge difference between these two, in fact they are the opposite sides
of a coin. By choosing class and variable names we communicate intend. If this intend
is not met you get confused. This type of miscommunication is exactly where I am loosing
time when developing. 

Talking about HSEARCH-1356 and visitors. Calling a base class XVisitor but then calling a subclass YStrategy is
highly confusing. Design patterns are there so that we can drop a name and have an immediate picture
what the code is doing. It boggles my mind what a StrategyVisitor is doing. It might be that the code is working,
but if nothing else we should stay away from using pattern names in this case. 

HSEARCH-1355 and HSEARCH-1356 are not about perfection, but about letting the code make sense and 
avoiding lost developer time by trying to understand miscommunicated intends.

Are these changes useful?  Heck they are! 


--Hardy