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.

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

Port to Python3 #59

Merged
merged 2 commits into from
Oct 5, 2015
Merged

Port to Python3 #59

merged 2 commits into from
Oct 5, 2015

Conversation

wazari972
Copy link
Contributor

Hello,

I've ported orochi code to Python3. It passes the testsuite (after a minor fix on the testsuite itself).

I'm now using it at home, I'll report and fix the bugs I may encounter with this new version.

Regards,

Kevin


Tested with Python 3.5.0.

The port mainly consists in handling correctly byte-string and
unicode strings. File buffering is also handled a bit differently,
and a race condition on player termination appeared in my PY3
environment but on in PY2.

@@ -256,9 +266,9 @@ def __reader(self, collector, source):
while True:
data = os.read(source.fileno(), 65536)
self.__lock.acquire()
collector.append(data)
collector.append(data.decode("ascii"))
Copy link
Owner

Choose a reason for hiding this comment

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

Why not append the .decode directly to the os.read call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're totally right, I've put out of the locked zone, and it would have saved me a bug investigation; because data == "" could not be True when data was bytes, so the thread never quit and I had 100% usage when stopping the player!

I've fixed that and squashed the commits.

@dbrgn
Copy link
Owner

dbrgn commented Oct 4, 2015

This is great, thanks a lot!

Should I merge this now, or would you prefer to test it for a while?

@wazari972 wazari972 force-pushed the master branch 2 times, most recently from 66be689 to 65c6ce9 Compare October 4, 2015 16:06
Tested with Python 3.5.0.

The port mainly consists in handling correctly byte-string and
unicode strings. File buffering is also handled a bit differently,
and a race condition on player termination appeared in my PY3
environment but on in PY2.
Result return by subprocess is in bytes in PY3.
Works also in PY2.
@wazari972
Copy link
Contributor Author

Should I merge this now, or would you prefer to test it for a while?

it seems fine to me, I've tested the CLI functionalities, so "normal" situations are okay. Hopefully there are no abnormal situation*, so you can merge.

(* I saw once a self.status['track']['url'] throwing a KeyError and crash Orochi, but that was with PY2, and I didn't note the faulty mix...)

@dbrgn
Copy link
Owner

dbrgn commented Oct 4, 2015

Thanks a lot for your help :) If you see anything else (or would even like to give the mpd port (#45) a stab 😉), feel free to send a PR!

@dbrgn dbrgn closed this Oct 4, 2015
dbrgn added a commit that referenced this pull request Oct 4, 2015
@wazari972
Copy link
Contributor Author

Great, thanks :) It's a nice tool you've developed, with good service provider behind, so I think I'll use it a lot, and I'll report/fix problems on the way! I'll give a try to the MPD port as well.

@wazari972
Copy link
Contributor Author

Danilo, you added me as author and close the PR ... but didn't merge it ... is it on purpose ... ?

@dbrgn dbrgn reopened this Oct 5, 2015
@dbrgn
Copy link
Owner

dbrgn commented Oct 5, 2015

😳 sorry, that was a mistake!

dbrgn added a commit that referenced this pull request Oct 5, 2015
@dbrgn dbrgn merged commit 38bd7af into dbrgn:master Oct 5, 2015
@dbrgn
Copy link
Owner

dbrgn commented Oct 5, 2015

Now! :) Thanks!

@dbrgn dbrgn mentioned this pull request Jan 12, 2016
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.

2 participants