On 10 July 2013 13:00, Emmanuel Bernard <emman...@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 _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
