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

Python 3 Support #81

Merged
merged 10 commits into from
May 2, 2017
Merged

Python 3 Support #81

merged 10 commits into from
May 2, 2017

Conversation

bemcdonnell
Copy link
Member

I've got a first iteration here for Py34 support on Windows. If you guys could check out my branch and do some testing, that would be nice.

This covers #80

@bemcdonnell bemcdonnell added this to the v0.4 milestone Mar 18, 2017
@bemcdonnell bemcdonnell self-assigned this Mar 18, 2017
@bemcdonnell
Copy link
Member Author

@michaeltryby, I'm finally coming out of the stone ages ;-)

@goanpeca
Copy link
Contributor

@bemcdonnell why 3.4? we should aim for either 3.5 or 3.6

@goanpeca
Copy link
Contributor

The changes you made to utf8 encode are not robust enough if the plan is to support both python2.7 and 3. Take a look at six library and use that please.

@bemcdonnell bemcdonnell changed the title Python 3.4 Support Python 3.6 Support Apr 30, 2017
@bemcdonnell bemcdonnell changed the title Python 3.6 Support Python 3 Support Apr 30, 2017
@bemcdonnell
Copy link
Member Author

@goanpeca, I updated the encoding behavior to use the python six module. Also, testing for 2.7, 3.4, 3.5, and 3.6 are all a pass! Let me know if you approve of these changes.

@goanpeca
Copy link
Contributor

goanpeca commented May 1, 2017

Hey @bemcdonnell looking much better

Still not sure about the next = __next__

if self._cuindex < self._nLinks:
linkobject = Link(self._model, self._linkid)
self._cuindex += 1 # Next Iteration
return linkobject
else:
raise StopIteration()

next = __next__ # Python 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

So as I understand, the method __next__ was day lighted in Python 3 pep3114. In py2, by default, the iterator calls next() therefore, I had to add this line in order for it to step.

I could use the 2to3 package but that doesn't feel like progress to me. If you know of a better way, please enlighten me!

Copy link
Contributor

Choose a reason for hiding this comment

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

six.next(<iterator>) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean remove those redefinitions and use directly six.next does not that work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not having any success with your suggestion. This code porting guide shows the iterator next implementation as I have it above: Conservative Python 3 Porting Guide (see the end). Maybe there is still a better way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok yes you are right :-)

@goanpeca
Copy link
Contributor

goanpeca commented May 1, 2017

@bemcdonnell we need to update the dependencies on setup.py to include six now.

@bemcdonnell
Copy link
Member Author

@goanpeca, shall I merge?

@bemcdonnell bemcdonnell removed the request for review from michaeltryby May 2, 2017 00:00
@goanpeca
Copy link
Contributor

goanpeca commented May 2, 2017

yeah ;-)

@bemcdonnell bemcdonnell merged commit a823f1c into pyswmm:master May 2, 2017
@bemcdonnell bemcdonnell deleted the py34 branch May 2, 2017 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants