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

WIP: Make flanker compatible with python 3 #106

Closed
wants to merge 34 commits into from

Conversation

salehe
Copy link

@salehe salehe commented Nov 3, 2015

Work In Progress

Make flanker compatible with python 3 (while maintaining python 2 compatibility)

Depends on mailgun/expiringdict#15 and mailgun/dnsq#7.

@mailgun-ci
Copy link

Can one of the admins verify this patch?

@salehe
Copy link
Author

salehe commented Nov 3, 2015

We are making an effort to make flanker compatible with python 3. We prefer to avoid making a python 3 only fork and in agreement with this comment, we prefer to pull the result in mailgun's upstream. This way, flanker will be compatible with both python 2 and 3.

We plan to use the excellent 'six' library and maybe 'python-future' for cross compatibility. This will have some cons like a little less code readability, but I don't think it will introduce any regression or performance difference.

As this will be a rather big pull request, I will be glad to know mailgun's opinion about all this beforehand.

@michi88
Copy link

michi88 commented Nov 16, 2015

@salehe I can also help out if you want and yes it would be awesome to know what @mailgun-ci wants beforehand.

@jessespears
Copy link
Contributor

Although we are a Python2.7 shop, and foresee being one indefinitely, I would support a Python3 PR. Thanks for the contribution and good luck!

@salehe
Copy link
Author

salehe commented Nov 17, 2015

@michi88 The hardest (and it has turned out to be very hard!) part is to realize intended types of strings in the library. The rest will be trivial using 2to3 and modernizer.

My approach is to analysis the types some interesting expression take while running the test suite (take a look at https://github.com/BayanGroup/flanker/tree/strs) and do logical reasoning about the code using its semantics, specs and data flow. Then I try to do the following without breaking tests:

  • Explicitly annotate all strings related to core functionality (all string should be either b'' or u'')
  • Explicitly annotate all regular expressions and set their flags
  • Use adjustments and type conversions when it's needed and is appropriate

I believe the addresslib library and utils.py are complete now. I'm moving to addresslib tests and the the mime library next.

@michi88
Copy link

michi88 commented Nov 18, 2015

@salehe Cool, I'm basically helping to move all projects I use to py3. First I need to help out at https://github.com/xhtml2pdf/xhtml2pdf as that is of higher priority for me. Will checkin here after that.

@edwardotis
Copy link

@salehe
The travis build is failing because mailgun team won't release their python 3 fix for expiringdict to pypi, nor will they remove it from flanker, even though it's an obsolete dependency .

Regardless, if you change your fork's requirements to add this commit of expiringdict, it will fix the current problem with the build in python 3.

#expiringdict required by flanker, and release version of expiringdict doesn't support python3
-e git+https://github.com/mailgun/expiringdict.git@bc21855fb8a51591a1b917cc82ba2e54805616c3#egg=expiringdict

salehe added 22 commits December 5, 2015 14:14
This way, we are not trying to eq check incompatible types, or call re.match on incompatible types

This had some implications on parsing (URL/UNI_URL and WHITESPACE/UNI_WHITE)
@kdeldycke
Copy link

Any news on that one?

@mailgun-ci
Copy link

Can one of the admins verify this patch?

@salehe
Copy link
Author

salehe commented Dec 19, 2016

Hi @kdeldycke

There is a py3partial branch that I've been working on with another approach for code changes. My original plan was to replace this PR with changes from that branch. Unfortunately I haven't made any progress in last few months.

The reason is that we've migrated what is needed for flanker to be in functional parity with py3's own email module and that has lead to large code changes that I'm afraid is nearly illogical for mailgun to accept in this repo. There are some small differences (like '\n' vs '\r\n') that may break mailgun's codes and also it's not clear what is the correct approach for them.

We've been using py3partial branch in our production system (not very large) with no problems. We have extensively used addresslib and mime modules except threading and dkim codes with no problems. You may evaluate it for your own use of-course.

I've been trying to not break original py27 tests (running on py27 interpreter) while making migrated tests pass on both py27 and py3 and except a few tests related to small differences that I mentioned, most of them are passing.

@kdeldycke
Copy link

Thanks @salehe for your report on the current situation!

@djstein
Copy link

djstein commented Jun 7, 2017

@salehe is it possible that this can be published to a alpha/beta PyPi repository?

@horkhe
Copy link
Member

horkhe commented Sep 11, 2017

@salehe do you still want to proceed with this? We are ok to accept it, after a review, as long as none of the existing tests break for python 2.7.

@horkhe
Copy link
Member

horkhe commented Apr 11, 2018

Obsolete by #193

@horkhe horkhe closed this Apr 11, 2018
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.

8 participants