Skip to content

Python3 #66

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

Closed
wants to merge 3 commits into from
Closed

Python3 #66

wants to merge 3 commits into from

Conversation

goochjj
Copy link

@goochjj goochjj commented May 4, 2019

Update code and Dockerfile for python 3

@goochjj goochjj mentioned this pull request May 4, 2019
@vl-homutov
Copy link
Contributor

Thank you for looking into this.
Will this change break things for those who don't have python 3? This is something I'd like to avoid when adding support for python3.
To accept this, we need to review currently supported OSes/packages, so it will take some time.

@goochjj
Copy link
Author

goochjj commented May 6, 2019

Yes, the library names changed, as did base64's handling of strings vs bytes.

When I wrote it in my original branch, I named the python3 version "nginx-ldap-auth-daemon3" instead... For the purpose of the PR it made sense to show the differentials. That could be how you support both - have the user select the version of the script for the version of python.

Sadly my python experience is not advanced enough for me to know how to do both simultaneously, but I think that'd just make the code unreadable anyway - python 3 is the future, python2 is deprecated as of next year - clearly python3 is the future, so supporting python 2 should be a secondary goal. If it were me I'd deprecate the python 2 version and give people type to move to the python 3 version, especially given how easy it is to do virtualenv. And the dockerfile should probably be python 3 because why not.... if the dependencies are already encapsulated then 2 vs 3 doesn't matter to the end user.

@vl-homutov
Copy link
Contributor

Python3 support was added. If you feel there is something missing, please create new PR against current master.

@vl-homutov vl-homutov closed this Oct 31, 2019
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