Refactored for Python3 compatibility #15
Conversation
scrapy_jsonrpc/jsonrpc.py
Outdated
| @@ -58,10 +58,11 @@ def jsonrpc_server_call(target, jsonrpc_request, json_decoder=None): | |||
| json_decoder = ScrapyJSONDecoder() | |||
|
|
|||
| try: | |||
| req = json_decoder.decode(jsonrpc_request) | |||
| req = json_decoder.decode(jsonrpc_request.decode()) | |||
There was a problem hiding this comment.
.decode() is almost always wrong - it decodes using sys.getdefaultencoding(), which can vary across machines, and which is often ascii. I think it is better to decode from UTF-8 because this is what JSON standard requires.
candale
force-pushed
the
python3_compatible
branch
from
bee6473
to
259162f
Compare
candale
force-pushed
the
python3_compatible
branch
from
259162f
to
40d4ce3
Compare
|
I've also overridden the |
|
@kmike Any other observations on this PR? |
|
This works well for me with python3, no issues encountered |
|
Can we merge it? |
|
Hi @ChiraMircea can you guys write a test for this? It's extremely hard to reason on your code without tests. |
dariuschira
force-pushed
the
python3_compatible
branch
from
6fe00bd
to
13fe000
Compare
|
Hey @sibiryakov I added a few basic tests and py37 to tox, hope it's enough, please let me know if you think more tests are required. |
Refactored the code so that is Python3 compatible. It mostly had trouble when dealing with bytes and returning keys from dictionaries.
I also added
objectin the inheritance forJsonResourceandWebservicebecause their base classes did not do, which required the call to the unbound__init__method of the parent.This should solve, at least partially, issue #12 .