That hasn't been common practice, but I guess it couldn't hurt in the future if you so chose.
On Thu, Oct 9, 2008 at 12:18 PM, Michael Hermanto <[EMAIL PROTECTED]>wrote: > In the spirit of retaining old reviews, will naming the attachment files > accounting the review number (ex: fix-624-bug-1, fix-624-bug-2, ...) help? > (Sorry, I am new to all this.) > > > On Thu, Oct 9, 2008 at 12:08 PM, John Hjelmstad <[EMAIL PROTECTED]>wrote: > >> Thanks for the reminder, Evan. Due to limitations with the code review >> tool, I had to delete the old reviews as new patches came in. The last, >> applied review was:http://codereview.appspot.com/7447/show >> >> --John >> >> >> On Thu, Oct 9, 2008 at 12:02 PM, Evan Gilbert <[EMAIL PROTECTED]> wrote: >> >>> Not sure if it is purposeful, but this review is no longer available for >>> viewing. Even if we upload a new patch or commit, this is still useful for >>> people looking back at the patch review. >>> >>> Evan >>> >>> >>> On Wed, Oct 8, 2008 at 1:14 PM, <[EMAIL PROTECTED]> wrote: >>> >>>> Thanks for the thorough review. New file attached to the issue. >>>> >>>> >>>> http://codereview.appspot.com/7441/diff/1/3 >>>> File >>>> >>>> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java >>>> (right): >>>> >>>> http://codereview.appspot.com/7441/diff/1/3#newcode155 >>>> Line 155: } >>>> On 2008/10/07 23:44:13, johnfargo wrote: >>>> >>>>> space between methods and between members and ctor >>>>> >>>> >>>> Done. >>>> >>>> http://codereview.appspot.com/7441/diff/1/2 >>>> File >>>> >>>> java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/RpcServletTest.java >>>> (right): >>>> >>>> http://codereview.appspot.com/7441/diff/1/2#newcode74 >>>> Line 74: assertDoGetWithoutHandler(null, "function"); >>>> On 2008/10/07 23:44:13, johnfargo wrote: >>>> >>>>> It looks like these don't actually perform the test you want -- you'll >>>>> >>>> need to >>>> >>>>> run EasyMock.verify(response) on the ServletResponse object you're >>>>> >>>> creating for >>>> >>>>> each test. >>>>> >>>>> You could probably unroll the test helper methods too, since they're >>>>> >>>> not >>>> >>>>> repeatedly used too much. Thus testDoGetNormal would look like: >>>>> request = createMockRequest(req, callback); >>>>> response = createExpectedResponse(string, code); >>>>> expect(handler.stuffHappens); >>>>> servlet.doGet(req, response); >>>>> verify(response); >>>>> >>>> >>>> Done. Should be all fixed now. >>>> >>>> http://codereview.appspot.com/7441/diff/1/2#newcode90 >>>> Line 90: int httpStatusCode) throws Exception { >>>> On 2008/10/07 23:44:13, johnfargo wrote: >>>> >>>>> args on the first line, wrapped 4-indented after >>>>> >>>> >>>> Done. >>>> >>>> http://codereview.appspot.com/7441/diff/1/2#newcode97 >>>> Line 97: EasyMock.replay(request, response, handler, writer); >>>> On 2008/10/07 23:44:13, johnfargo wrote: >>>> >>>>> stylistically we import static on these eg. >>>>> import static ...EasyMock.expect; >>>>> >>>> >>>> Done. >>>> >>>> http://codereview.appspot.com/7441/diff/1/2#newcode106 >>>> Line 106: int httpStatusCode) throws Exception { >>>> On 2008/10/07 23:44:13, johnfargo wrote: >>>> >>>>> args on the first line, wrapped 4-indented after >>>>> >>>> >>>> Done. >>>> >>>> http://codereview.appspot.com/7441/diff/1/2#newcode137 >>>> Line 137: String contentType, int httpStatusCode) throws IOException { >>>> On 2008/10/07 23:44:13, johnfargo wrote: >>>> >>>>> This can be called createHttpResponse (and perhaps used for POST tests >>>>> >>>> later) >>>> >>>> Done. >>>> >>>> >>>> http://codereview.appspot.com/7441 >>>> >>> >>> >> >
