[vtk-developers] KdTree FindPointsWithinRadius

[David Gobbi]

On Thu, Sep 23, 2010 at 12:42 PM, David Doria <daviddoria at gmail.com> wrote:
> On Wed, Sep 22, 2010 at 10:45 AM, pat marion <pat.marion at kitware.com> wrote:
>> Are you talking about pushing your commits 26d55f7a59 thru bb4b221faf2?  My
>> opinion is that these commits are not ready to be pushed, but I have not had
>> a lot of time to do a proper review.  For ease of reviewing, I'd recommend
>> squashing all your commits into two- the first commit performs adds your new
>> feature, the second commit that adds the tests.  Then use git format-patch
>> and attach the commits to the mailing list.
>>
>> Pat
>
> Yes, those are the commits. I did some reading about squashing etc. I
> see how to combine commits, but I don't know how to separate commits.
> That is, in most of the commits I worked on the implementation as well
> as the test. How would I break these apart?
>
> Also, why would you want to use git format-patch? Isn't github's
> interface exactly for this type of review? And the idea of publishing
> a branch instead of just the files is that a review can clone the
> branch and try everything out right away, right? And githubs red/green
> highlighted lines as well as displaying individual files also solves
> the problem of having the test and the implementation in the same
> commit, right?
>
> It just look to me like this process of preparing the code for a
> review will take just as much time as writing these tiny patches. Is
> this the "agreed upon" procedure? I know ITK has been using Gerrit and
> I know we're not there yet, but would most people know what to do with
> a git format-patch email?

If you want code to be reviewed, you must make the code easy to
review.  Give an http link to github that shows the changes as a
single diff.  Or provide a singe patchfile in "unified" style (though
a github link would be preferable to me, at least).

But in my opinion, interface methods like this should be added to
vtkPointLocator, not to its subclasses.  But even then, the small
convenience that this method adds is not enough for me to approve of
its addition.

  David