Code review comment for lp://staging/~ceronman/txsolr/response-consumer-errback-843429

Revision history for this message
Jamu Kakar (jkakar) wrote :

[1]

+ def testResponseConsumerWithGoodResponse(self):
+ """
+ The L{ResponseConsumer} protocol should fire the given L{Deferred} if
+ the given body is a valid Solr response.
+ """

and

+ def testResponseConsumerWithBadResponse(self):
+ """
+ The L{ResponseConsumer} protocol should fire the given L{Deferred} with
+ an C{errback} with L{SolrResponseError} if the decoding fails.
+ """

Please s/L{Deferred}/C{Deferred}/ in these docstrings.

[2]

+ self.assertEqual(solrResponse.header['status'], 0)
+ self.assertEqual(solrResponse.header['QTime'], 2)
+ self.assertEqual(solrResponse.results.numFound, 0)
+ self.assertEqual(len(solrResponse.results.docs), 0)

Please reverse the order of the arguments here.

[3]

One minor thing:

    def __init__(self, deferred, responseClass):
        self.body = ''
        self.deferred = deferred
        self.responseClass = responseClass

    def dataReceived(self, bytes):
        _logger.debug('Consumer data received:\n' + bytes)
        self.body += bytes

It's better to append the bytes to a list and use ''.join(self.bytes)
when you want the body. Appending to the string will use a lot of
memory unnecessarily.

Anyway, this looks good, +1!

review: Approve

« Back to merge proposal