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

python3 has no unicode builtin #31

Closed
wants to merge 7 commits into from
Closed

Conversation

eddy-geek
Copy link

This fixes python3 compatibility (at least for getting events in date range as per the tutorial)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) when pulling d126b5d on eddy-geek:master into b57555d on linkedin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) when pulling f8fc2e4 on eddy-geek:master into b57555d on linkedin:master.

@@ -7,6 +7,10 @@
from lxml.builder import ElementMaker
from ..utils import convert_datetime_to_utc

import sys
if sys.version_info[0] == 3:
unicode = str
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, thanks for the commit! Can you tell me a little more why this was necessary? What broke?

Copy link
Author

Choose a reason for hiding this comment

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

The doc states "PyExchange supports Python 2.6 and 2.7. Our code is compatible with 3.3+, but see the notes below on getting it working."
Besides the fact that the "notes" on ntlm are not valid anymore (pip3 install pyexchange now works out of the box AFAIK), running the this code from the docs under python3 will fail with NameError: name 'unicode' is not defined or similar error.
Maybe this would be more visible if python3 was enabled on Travis C.I./coverage ?

The culprit is in get_calendar_item around line 130

       u'MaxEntriesReturned': unicode(max_entries),

BTW, same issue in exchange2010/init.py in move_to with:

    if not isinstance(folder_id, basestring):

I will fix that one too, probably along with some mail support :-)
Do you prefer separate PRs ?

@catermelon
Copy link
Contributor

Derp, I meant to get back to you and blanked. Sorry for the delay! You're right, let me enable Python 3 support on Travis so I can see what breaks across the board.

@catermelon
Copy link
Contributor

It looks like I had some unmerged code that fixes all this - can you try the most recent commit I just made and see if it works for you? It's passing Travis now for 3.3 and 3.4.

thanks again for the work, I appreciate it.

@eddy-geek
Copy link
Author

Not tested yet, but this looks to be removing all the unicode & basestring references that were causing problems. Thanks.
I didn't create the PR from a feature branch so all my new commits are creeping in.

  • Do you want to close this one ?
  • Should I create a new one ? I have started basic mail support but there are no unit-tests, so not sure if you would want to merge it. Other changes should be self-explanatory.

@catermelon
Copy link
Contributor

If you wouldn't mind starting a new one, that would be fab. And yes, please, I would need some tests before merging it, although I'm happy to look at a preliminary pull request if you want another eye for code review.

@eddy-geek eddy-geek closed this Mar 28, 2022
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

3 participants