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.