search

grnet / djnro

DjNRO hits the decks of eduroam database management
http://djnro.grnet.gr/
Other
10 stars 21 forks source link

Python3 compatibility - preserving Python2 #64

Closed vladimir-mencl-eresearch closed 5 years ago

vladimir-mencl-eresearch commented 5 years ago

Hi @zmousm ,

This is my first cut of the Python3 overhaul.

I was in principle following the general 2-3 porting guidelines (preserving 2 compatibility).

And the Django porting documentation, https://docs.djangoproject.com/en/1.11/topics/python3/

We are at the right spot to do the porting: the first Django LTS to support Python3, and the last to still support Python2...

I've documented the rationale for changes done in the commit messages - hope all is clear.

I got the code to the point where everything works with Python3 (tried all URLs, management commands) and still also works under Python2.

Please let me know what you think about it.

Cheers, Vlad

PS: And thanks for the hint on the sorting issue - I've cherry-picked your commit and included it here..

vladimir-mencl-eresearch commented 5 years ago

Hi @cangus , would you be able to review this PR? Thanks a lot in advance! Cheers, Vlad

cangus commented 5 years ago

Looks good, considering the bulk of the changes do not affect actual behaviour and serve to support Python 3 I don't have any comment regarding approach.

In other code bases I have seen migrating from py2 to py3 runtime string encoding issues pop up a lot. It looks like you have this covered, but testing with some strings that contain characters that live at code points outside of ascii might be an idea if you have not already.

I do note that there is a mix of approaches to string formatting (string concatenation and %s style interpolation), often in the same function which is inconsistent. This is probably best approached as a seperate PR as what you have here appears to be a clean, minimal change set required to support py3.

vladimir-mencl-eresearch commented 5 years ago

Hi Culley,

Thanks for the review. I was testing this on our internal database which does contain non-ASCII characters (Te Whare Wānanga o Awanuiārangi), and I've been using this as a test-case for proper handling of unicode - works for both Python2 and Python3 deployments.

If this satisfies your review, I'd merge the PR - and we can later look at other suggested improvements to the code base.

Cheers, Vlad