Change in vdsm[master]: stomp: parsing buffer refactoring

fromani at redhat.com fromani at redhat.com
Thu Mar 12 13:09:43 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: stomp: parsing buffer refactoring
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

mostly questions, hence -1 for visibility, but quite some of them

https://gerrit.ovirt.org/#/c/38666/1/lib/yajsonrpc/stomp.py
File lib/yajsonrpc/stomp.py:

Line 165:     def _flush(self):
Line 166:         self._buffer = ""
Line 167: 
Line 168:     def _write_buffer(self, buff):
Line 169:         self._buffer += buff
so we are going to append data to a string, which is usually bad practice in python.

Which is the reason for this change?
Line 170: 
Line 171:     def _get_buffer(self):
Line 172:         return self._buffer
Line 173: 


Line 172:         return self._buffer
Line 173: 
Line 174:     def _handle_terminator(self, term):
Line 175:         res, _, rest = self._buffer.partition(term)
Line 176:         if not _:
well, if we do logic on this field, then we should give a name to it.

The convention is that we name '_' for field we don't care about, but here we do!
Line 177:             return None
Line 178: 
Line 179:         self._buffer = rest
Line 180: 


Line 396: 
Line 397: class AsyncDispatcher(object):
Line 398:     log = logging.getLogger("stomp.AsyncDispatcher")
Line 399: 
Line 400:     def __init__(self, frameHandler, bufferSize=4096):
ok, but why? Only for performance?
Line 401:         self._frameHandler = frameHandler
Line 402:         self._bufferSize = bufferSize
Line 403:         self._parser = Parser()
Line 404:         self._outbox = deque()


Line 573: 
Line 574:         return subscriptionID
Line 575: 
Line 576: 
Line 577: class Subscription(object):
is this used elsewhere outside this module? If not, what about making it _private?
Line 578:     def __init__(self, client, subid, ack):
Line 579:         self._ack = ack
Line 580:         self._subid = subid
Line 581:         self._client = client


-- 
To view, visit https://gerrit.ovirt.org/38666
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b835e0d8a5ca20e67f0562955255ba1ad6ae9a1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list