Add support for json messages and no compression #8
Conversation
| self.decompress_fun = zlib.decompress | ||
| def __init__(self, encoding=None, decompress=None, msgformat=None): | ||
| if decompress: | ||
| import zlib |
There was a problem hiding this comment.
why lazy import of zlib? It is a standard library.
There was a problem hiding this comment.
sure, i can import it, there is no point in polluting the global namespace unnecesarily but it wouldnt be a big deal.
| def get_kafka_msg_samples(msgs=None, fetch_count=0, msgformat='msgpack', compress=True): | ||
| if not msgs: | ||
| msgs = [('AD12345', "my-body"), | ||
| ('AD34567', "second my-body"), |
There was a problem hiding this comment.
Can you separate pylint style fixes and new feature in different commits?
There was a problem hiding this comment.
oh ok, do you guys dont mind if there are multiple commits per PR? or should i remove it completely and then do another PR?
There was a problem hiding this comment.
I don't mind multiple commits per PR. Not needed an extra PR.
| def __init__(self, handlers_list, encoding=None, *args, **kwargs): | ||
| self._handlers = [] | ||
| self.processor_handlers = MsgProcessorHandlers(encoding=encoding) | ||
| self.processor_handlers = MsgProcessorHandlers(encoding=encoding, *args, **kwargs) |
There was a problem hiding this comment.
it is weird that positional arguments *args come after a key argument encoding=. Didn't pylint complain here?
There was a problem hiding this comment.
no, afaik this is perfectly valid. If it goes against SH style guide let me know and I can make it an unnamed argument (i always prefer using names for the shake of documenting but I see your point).
| self.__encoding = encoding | ||
| self.deserialize_fun = self.set_deserializer(msgformat) | ||
|
|
||
| def set_deserializer(self, msg_format): |
There was a problem hiding this comment.
in the way you are using this method, seems better a name like get_deserializer(). The method doesn't really set anything. Only returns an object than it is set in previous line.
There was a problem hiding this comment.
good point, naming is important
| for i in range(100, 200): | ||
| self.assertTrue('AD%.3d' % i not in msgsdict) | ||
|
|
||
| # def test_kafka_scan_resume(self, kafka_consumer_mock, batchsize=100): |
There was a problem hiding this comment.
please, don't remove this test.
There was a problem hiding this comment.
yeah this was an error.
There was a problem hiding this comment.
Actually, now that I see, this whole code block is commented in master. Are you sure you want to keep it in master? Saving code that you might use in the future is not good practice (YAGNI probably), and that is what git history is for.
There was a problem hiding this comment.
you are right. I hand't noticed this was commented test.
Rename set_deserializer to get_deserializer
manugarri
force-pushed
the
addParams
branch
from
3b81207 to
9013b69
Compare
|
@kalessin Fixed the changes, and commented on your requests. Let me know if you need anything else. We are using kafka scanners and I'd rather not keep our own fork when this change could benefit everyone instead. |
|
thanks @manugarri . I merged and updated/tagged version. New version just published into pypi. |
|
yaaay! Thanks @kalessin |
2 participants
This PR implements the following changes:
I set all the default encoding to
msgpackand the decompress toTrue. That should make it compatible with current use cases.