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@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev