I think we all agree that PRs have many merits: it's almost pointless to try highlighting them as we're all experienced developers, and I guess we all have some horror stories from back in the dark ages when we couldn't use them. When we moved to git very few of us had some experience with it, but those who had did propose a solid workflow; we're using this process now since approximately 2 years, and while I think it's been quite good, it's time to apply some Retrospective. I won't annoy you with the long list of benefits, far more interesting to discuss what I'm considering - in my experience - a strong drawback: ### Delays ### Pull requests introduce some delay; sometimes it's short and reasonable, sometimes it's huge and well needed, but sometimes it gets very long without providing a significant value to the project. Actually, since these delays can cost us significant time, there is a large opportunity cost and not all of us are always aware of what you might be blocking. # Context Switch # Now even if it introduces a strong delay, often it's not a big problem as we can "context switch" to other subjects or even other projects, but in practice I'm struggling a lot by this context switch and - assuming I'm not the only one hit by this - I think we should avoid it as much as possible: context switch is not just energy draining, it's also demotivating and has a direct impact on the code quality. That's huge: code quality is the core of what we're trying to improve with the pull requests process, but in practice it's achieving the opposite! # Real tangible delays # In some cases, a failed review doesn't just force a context switch but brings to failed deliveries, or unability to work on the next task. Ok this is not common, but it happens. # Conflicts # This is not a soft-problem nor too hard to handle, but of course the more we keep parallel branches the more we have conflicts to resolve. Some of you pointed out it's no big deal but I've often volunteered for some hard ones... I guess I should change that strategy so you can better feel the pain ;-) ### Counter-measures ### I don't think it's worth cancelling the PRs process as there are great benefits, but it's worth exploring how to minimize the undesired side-effects. 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. 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". 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 to make sure that we're spending time on the highest value instead of entertaining ourselves with design puzzlers.. I know the feeling, we all are scientists and enjoy some nitty gritty polishing but often there's no real gain from it, not even long term readability as I'd rather hope our code to evolve faster.. we're not polishing a statue of immutable stone. Also, a follow-up improvement can easily get reassigned to someone else; for example often the contributor pointing out a possible improvement has a better idea of the change he would like to see applied, so he could "just code it" and propose it as a change. Finally, we could try to *reject* PRs more often. If you think there is a blocking problem with the pull, today we would usually ask for improvements and wait for the puller to fix, we could as well make it very clear when we think that it's a blocker problem by rejecting it.. I suspect that could have some good effects: - no push-force on the same pull hiding previous dicussions - potentially a different reviewer on the next attempt (we had cases on pulls gettting stuck because the single reviewer was away!) - no long lists of dormant pull requests I think this would also encourage people to be quick in merging if there is *any* open pull request. The problem here is to not take it as an offence, but I think if we were all doing it more regularly, - if it becomes a normal process rather than an exception - then there would be no problem with it. Possibly we would need to explain that nicely to newcomer contributors! # Avoid pull requests for previews This is becoming more frequent but I came to dislike pulls which stay open for long (days). I'd prefer to keep the queue empty and try some alternatives, like sending pull requests to specific developer's rather than to the main project. This way you could "invite" specific devs, and they could close the pulls to signal they consider the review task complete.. or explore something like that. # 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. I hope I've learned to "let go" a bit.. for these cases I think it's of course great to comment on github but use good judgment to see if it's worth delaying the merge. Often I find myself applying minor fixed myself, some other times, I expect follow up polishing pulls, or we open new issues. # When to delay? Consider that a sequence of PRs will show up in history as a sequence of commits, and it's going to be impossible to figure out from the master history if the changes are caused by a single PR or by multiple PRs. Consider also that when asking a PR improvement, we often stack additional commits... I really prefer if we all could "fixup" the wrong commits, but in case there is no need for it, you could as well merge and ask for further improvements.. could be a good rule of thumb? Let's fight our love for perfection and bring home some *useful* stuff ASAP. (Hint: renaming internal classes is not in this category) -- Sanne _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

> I also think opening issues to things that are not fully on the topic is > a good strategy to keep the cycle on a given PR short. 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.

>> 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.

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.

As much as I enjoy this discussion, right now we have 0 open pull request in Search. So where is the problem?