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 #43

Closed
wants to merge 4 commits into from
Closed

Python 3 support #43

wants to merge 4 commits into from

Conversation

reinhardt
Copy link

Refs #33

- fixed imports
- text/encoding fixes
- replaced iteritems
- fixed exception handling
- replaced print statement
- mangled doctests, added Py23DocChecker
Refs #33
- fixed boolean value of AccountExpired
- fixed more tests
Refs #33
@coveralls
Copy link

coveralls commented Feb 7, 2019

Coverage Status

Coverage decreased (-0.1%) to 94.373% when pulling db28b83 on python3 into b0cf548 on master.

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -334,16 +347,21 @@ def __call__(self):
self.changed = False
self._action = None
deleted = [self[key] for key in self._deleted_children]
for node in self.storage.values() + deleted:
for node in list(self.storage.values()) + deleted:
Copy link
Member

Choose a reason for hiding this comment

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

here I would prefer itertools.chain

@@ -1,13 +1,17 @@
# -*- coding: utf-8 -*-
from __future__ import print_function
Copy link
Member

Choose a reason for hiding this comment

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

I dont think this is necessary, `print('foo') works in python 2 OOTB

Copy link
Author

Choose a reason for hiding this comment

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

I think you're right. Probably inserted by modernize and missed during cleanup.


>>> str(filter)
'(|(cn=sepp)(sn=meier\xc3\xa4))'
>>> str(filter) in ['(|(sn=meierä)(cn=sepp))', '(|(cn=sepp)(sn=meier\xc3\xa4))']
Copy link
Member

Choose a reason for hiding this comment

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

I can not follow here? why check both 'ä' variants?

Copy link
Author

Choose a reason for hiding this comment

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

The representation differs in Python 2 vs. 3. We don't care what exactly it is, so we allow both.

Copy link
Member

@rnixx rnixx left a comment

Choose a reason for hiding this comment

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

Hi Manuel,

Thank you very much for your efforts.

It was my original plan to work on this topic on my own at alpine city sprint. Unfortunately I was not able to attend.

My plan was to convert doctests to unittests before porting to python3. This is the reason why I did not merge your PR into master.

Unittest conversion is done now with #44, and python3 support is in there as well.

I've taken some implementation details from your PR, thanks again. The biggest difference in my python 3 support implementation is the use of bytes_mode=False in python-ldap, which simplifies things a lot, because it now provides unicode support for for most of the data OOTB, which was handled by node.ext.ldap before.

I'll close this PR

@rnixx rnixx closed this Apr 15, 2019
@jensens jensens deleted the python3 branch April 9, 2020 16:23
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.

None yet

4 participants