2011/10/23 Tomaž Muraus <[email protected]>: > On Fri, Oct 21, 2011 at 8:36 PM, Caio Romão <[email protected]> wrote: <snip> >> >> Great, got it. I've updated the branch and created a proper pull >> request on github: >> https://github.com/apache/libcloud/pull/32 >> So now code review can flow through github -- or do you prefer another >> method? >> > > > Yeah a pull request is great for doing a review. Ideally when the review is > done and the branch is ready to be merged you would open a JIRA ticket and > attach a patch there. >
Thanks for the review! When the branch is ready to be merged I'll update the LIBCLOUD-97 ticket with the patch then. > >> >> > On Wed, Oct 19, 2011 at 2:33 PM, Caio Romão <[email protected]> wrote: >> <snip> >> >> Conjecturing: >> >> >> >> Is adding logic to the Response class the best way to do this kind of >> >> verification? >> > >> > >> > I think having error handling logic in Response class is OK. Adding >> another >> > class or something like that wouldn't really reduce the complexity or >> make >> > error handling code more simple or clear. >> > >> > I'm definitely open to better ideas though. >> > >> >> No better idea currently, I'll sleep on it and if I come up with >> anything feasible I'll bring the topic back up again. >> > > I just posted some comments on your branch. I would like to get those > addressed and then merge your patch into trunk. Overall it's an improvement > over our current code and it reduces line count for ~100 or so :) > > I've ack'ed your suggestion regarding the "parse_zero_length_body" suggestion and commented a bit: 1. Regarding slicehost driver "len(self.body) <= 1" test (suggesting to self.body.strip() somewhere) 2. Regarding adding a verification for the Content-Type header into {Xml,Json}Response classes (along with a toggle switch to avoid checking it)
