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

distributed: AstronInternalRepository et al is now compatible with Python 3 #30

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ksmit799
Copy link

This closes issue #23

- getMessage() returns binary data which implicitly casts to a string using Python2, but doesn't on Python3. A blob is identical to a string, but is version agnostic.
@drewc5131
Copy link

drewc5131 commented Jan 11, 2019

You are missing quite a bit needed to close that issue. For instance, you still have a ton of xranges, longs, and other things that are gone in python 3.

I have all of it fixed in my fork, however, it may screw up support with python 3, and the formatting of all the files got screwed up, so I probably won't make a pull request.

- xrange -> range.
- getString -> getBytes for appended data.
@ksmit799
Copy link
Author

@drewc5131 I've put through some more changes. Let me know if I've missed anything.

On another note, I currently don't have any readily available code to test Python 2 compatibility, so more obscure changes might mess something up (I'm on vacation currently).

@drewc5131
Copy link

Yeah the problem is i dont think these changes can be merged due to them possibly breaking python 2 support. However, I personally would allow them in if you added some if statements to help with not backwards compatible changes, but I can't say whether astron will allow them. Try adding

if sys.version_info >= (3, 0):
    # stuff that only works with python 3, not with 2

else:
    # stuff that only works with 2, not 3

I can see this being used for the get bytes (though that may not be required, not sure), and for the removal of the long type, which i can definitely recommend doing that for.

@ksmit799
Copy link
Author

Yeah, everything so far should be backwards compatible (range is slightly slower then xrange on Python 2). Just not sure about the (int, long) camparison, that will have to be tested.

@Astron Astron deleted a comment from drewc5131 Apr 13, 2019
@Astron Astron deleted a comment from nate97 Apr 13, 2019
@Astron Astron deleted a comment from ksmit799 Apr 13, 2019
@Astron Astron deleted a comment from nate97 Apr 13, 2019
@Astron Astron deleted a comment from nate97 Apr 13, 2019
@anonymous1234566
Copy link

I think breaking python 2 support is fine since python2 is no longer officially supported

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