Skip to content

Refactored for Python3 compatibility#15

Open
candale wants to merge 4 commits into
scrapy-plugins:masterfrom
candale:python3_compatible
Open

Refactored for Python3 compatibility#15
candale wants to merge 4 commits into
scrapy-plugins:masterfrom
candale:python3_compatible

Conversation

@candale
Copy link
Copy Markdown

@candale candale commented Jun 18, 2017

Refactored the code so that is Python3 compatible. It mostly had trouble when dealing with bytes and returning keys from dictionaries.

I also added object in the inheritance for JsonResource and Webservice because 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 .

Comment thread scrapy_jsonrpc/jsonrpc.py Outdated

try:
req = json_decoder.decode(jsonrpc_request)
req = json_decoder.decode(jsonrpc_request.decode())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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 candale force-pushed the python3_compatible branch from bee6473 to 259162f Compare June 19, 2017 06:31
@candale candale force-pushed the python3_compatible branch from 259162f to 40d4ce3 Compare June 19, 2017 07:42
@candale
Copy link
Copy Markdown
Author

candale commented Jun 19, 2017

I've also overridden the log method in WebService because it encoded the log line even if it was on Python3, which turned it into bytes.

@candale
Copy link
Copy Markdown
Author

candale commented Jun 26, 2017

@kmike Any other observations on this PR?

@dariuschira
Copy link
Copy Markdown

This works well for me with python3, no issues encountered

@dariuschira
Copy link
Copy Markdown

Can we merge it?

@sibiryakov
Copy link
Copy Markdown
Contributor

Hi @ChiraMircea can you guys write a test for this? It's extremely hard to reason on your code without tests.

@dariuschira
Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants