Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: code compatible with python2 and python3 #136

Merged
merged 7 commits into from
Nov 21, 2019
Merged

feat: code compatible with python2 and python3 #136

merged 7 commits into from
Nov 21, 2019

Conversation

CPtung
Copy link
Contributor

@CPtung CPtung commented Sep 24, 2019

No description provided.

@CPtung CPtung requested a review from taotao September 24, 2019 10:38
@CPtung CPtung requested a review from imZack September 25, 2019 02:06
taotao
taotao approved these changes Sep 26, 2019
View reviewed changes
Copy link
Contributor

@taotao taotao left a comment

Choose a reason for hiding this comment

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

LGTM
@imZack any comment ?

@imZack
Copy link
Contributor

imZack commented Sep 26, 2019

LGTM

However, are all the modules in the requirements support Python 3?

imZack
imZack approved these changes Sep 26, 2019
View reviewed changes
.travis.yml Outdated
@@ -6,7 +6,7 @@ install:
script: make tox
env:
- TOXENV=py27
- TOXENV=pypy
- TOXENV=py36
Copy link
Contributor

Choose a reason for hiding this comment

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

The modifications will make pypy failed? Otherwise, maybe we can just add one more entry without removing pypy

setup.py Outdated
@@ -29,6 +29,8 @@ def read(*paths):
'Programming Language :: Python :: 2',
'Programming Language :: Python :: 2.6',
'Programming Language :: Python :: 2.7',
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.6',
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI --

https://wiki.debian.org/Python
Debian 9 uses Python 3.5 and Debian 10 uses Python 3.7

@taotao
Copy link
Contributor

taotao commented Sep 27, 2019

LGTM

However, are all the modules in the requirements support Python 3?

@imZack I think the py36 test run pass, means the modules in requirements.txt support Python 3.

@imZack
Copy link
Contributor

imZack commented Sep 30, 2019

@taotao yes but something like paho-mqtt we probably need to check again. It's not in the test.

@taotao
Copy link
Contributor

taotao commented Oct 2, 2019

@imZack after testing with sanji-examples the hellosanji bundle can not register correct. (only show 1 registration DELETE message)

@taotao
Copy link
Contributor

taotao commented Oct 28, 2019

@imZack after testing with sanji-examples the hellosanji bundle can not register correct. (only show 1 registration DELETE message)

@imZack I found the root cause of this issue.
at: https://github.com/Sanji-IO/sanji/blob/develop/sanji/message.py#L123
the message type in python 2 is basestring(), but in python 3, it's type is bytes.
we need to add six.binary_type for message type checking.

I also find a issue, when we change the QoS from 2 to 0. (commit)
with sending sanji request too quickly, it will get 500 internal error.
do you know the reason that we change the QoS from 2 to 0 ?

@imZack
Copy link
Contributor

imZack commented Oct 29, 2019

@taotao Got it, yeah...six.string_types should work.

The reason I've changed default QoS from 2 to 0 is mainly due to client/broker are on the same host. Do you have the script to reproduce 500 error?

@taotao
Copy link
Contributor

taotao commented Oct 31, 2019

@imZack
You can try this flow:

  1. get a ThingsPro V2 device
  2. stop ThingsPro : mx-tp-ctl -e 0
  3. start mosquitto, sanji-controller: systemctl restart mosquitto sanji-controller
  4. update sanji sdk to v1.0.2: pip install --upgrade https://github.com/Sanji-IO/sanji/tarball/1.0.2
  5. clone sanji-examples: git clone https://github.com/Sanji-IO/sanji-examples.git
  6. run 01-hellosanji: cd 01-hellosanji && python hellosanji.py
  7. send sanji get request: mosquitto_pub -t "/controller" -m '{"id":2,"method":"get","resource":"/hellosanji"}'

@taotao
Copy link
Contributor

taotao commented Nov 15, 2019

@CPtung
please help to update source code that I mention at this comment, and also revert this commit to change qos from 0 back to 2.

@CPtung CPtung force-pushed the develop branch 3 times, most recently from 5463dea to 01d0bd9 Compare November 19, 2019 14:13
lwindg
lwindg reviewed Nov 20, 2019
View reviewed changes
tests/sanji/test_core.py Outdated
@@ -207,7 +207,7 @@ def test__dispatch_message(self): # noqa
self.test_model._Sanji__dispatch_message(m)
resp.assert_called_once_with(
code=404, data={"message": ANY})

Copy link
Contributor

Choose a reason for hiding this comment

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

extra space~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fault...

lwindg
lwindg reviewed Nov 20, 2019
View reviewed changes
sanji/core.py
@@ -134,8 +135,11 @@ def ___dispatch(handler, message):

try:
for result in results: # same route
map(lambda handler: ___dispatch(handler, result["message"]),
data = map(lambda handler: ___dispatch(
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need to get data and list it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Map in Python2 will return a list object but map object in Python3. It causes the lambda should execute the handler to do the dispatch but nothing happened. Here I add a workaround which casts the map object to list, then the result becomes what we expect.

@taotao taotao merged commit 6ed3020 into Sanji-IO:develop Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lwindg lwindg lwindg left review comments

@imZack imZack imZack approved these changes

@taotao taotao taotao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CPtung @imZack @taotao @lwindg