I think what happened with this change is completely acceptable.
Given my limited bandwidth I can't review every change as it comes
in. I'm sure there are plenty of people in my situation as well.
Given that issues are in JIRA and they're referenced in checkins I'm
happy.
For largish changes I'm happy to see code review, since that has
proven successful in improving quality.
On Feb 21, 2009, at 3:23 AM, Ian Boston wrote:
(I am asking these questions to get a common understanding of how we
work as a community, not because I feel I am right or wrong)
I agree, people caring, and reviewing the code before/after and long
after commit is 100% what this community should be doing. No
argument there, what I want to know is how we achieve that...
because at the moment we have 2 processes for code review and its
confusing.
So, neither I nor Ben pushed the patch to the appspot codereview.
(not least because my google id is not subscribed to shindig-dev and
for some reason I cant make aliases work properly with google id's)
but the patch was pushed into JIRA and then discussed at some length
on the list, ('Extension to getPerson for
0.9',SHINDIG-904,SHINDIG-918,'PersonService precludes filtering'),
starting on 5 Feb.
IMHO, that was review?
If patch + jira is not going to considered sufficient for code
review then IMHO we need to make that clear so decisions like the
one I made don't go un-noticed by those with production
implementations until they are found in production.
BTW, I was thinking of those who had already implemented the SPI in
making the decision, but obviously I was not privy to their source
code.
I have no preference to how we work, either way is good with me, but
I don't want to be making decisions totally on my own, just because
one half of the community is working one way, and the other are
working another.
Ian
(also IMHO, just because something was reviewed doesn't mean that
its perfect and cant be made better in the future and always the
case with anything I write :))
On 20 Feb 2009, at 18:22, Adam Winer wrote:
As for the commit: In the past, we have reviewed patches in JIRA,
talked
about them and the committed them.
Have we changed the process to have all patches go into code
review before
being comitted ?
I dont remember seeing any conclusions about this on list?
No, there isn't any such requirement. Checking in without a review
means
there's a greater chance of someone griping after the commit, which
is a bit
more expensive for all concerned. That's what's happening now -
which is
all fine, and perfectly normal. The fact that people care is a
Good Thing
for the health of the project.
