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

Merged
merged 12 commits into from Aug 11, 2014
Merged

Python3 #103

merged 12 commits into from Aug 11, 2014

Conversation

@sloria
Copy link
Contributor

@sloria sloria commented Jul 4, 2014

Fixes a number of Python 3 incompatiblities (resolves #51 ). This patch tests against py26, py27, py33, and py34. All tests are currently passing on tox.

@tomchristie
Copy link
Contributor

@tomchristie tomchristie commented Jul 7, 2014

Looks good!

Loading

binary_type = bytes
string_types = (str,)
unicode = str
basestring = (str, bytes)
Copy link
Contributor

@tomchristie tomchristie Jul 7, 2014

Choose a reason for hiding this comment

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

I'd prefer if we named this module compat.py - drop the leading underscore.

Loading

@Hernrup
Copy link
Contributor

@Hernrup Hernrup commented Jul 7, 2014

This is great! However I feel that there is a strong chance that some of the older PRs will break this compatibility again. Personally I feel that is it a little hard to contribute at the moment as the branches has diverged allot.

I think this project would get allot more forward momentum if we could get some cleanup on the PRs. Maybe its even better to pull few almost-done once to get back to a steady state to branch from.

What do you think @tomchristie? I know you have allot to do but could we perhaps get a fast cleanup weekend going?

Loading

@tomchristie
Copy link
Contributor

@tomchristie tomchristie commented Jul 7, 2014

@Hernrup If anyone else is around at EuroPython, might be abel to find some time then?

Loading

@sloria
Copy link
Contributor Author

@sloria sloria commented Jul 8, 2014

OK, made the style fixes you suggested. Let me know if there's anything more to do.

Loading

@Hernrup
Copy link
Contributor

@Hernrup Hernrup commented Jul 10, 2014

Turns out there are a few packages that has been renamed/changed in py3. The one with the largest impact is probably "SimpleHTTPServer".
I dont have the knowledge to investigate these py3 issues. Do you think you could have a look at that as well in this PR @sloria? Would be very nice to get the py3 support going.

Loading

@sloria
Copy link
Contributor Author

@sloria sloria commented Jul 11, 2014

Yep, upon actually trying to run the mkdocs CLI on Py3, I found a few other incompatibilities. I will look these over.

Loading

@sloria
Copy link
Contributor Author

@sloria sloria commented Jul 11, 2014

OK, the remaining incompatibilities have been resolved.

Loading

@Hernrup
Copy link
Contributor

@Hernrup Hernrup commented Jul 11, 2014

Very nice! Works like a charm.

Not to push my luck but do you have any suggestions for what to do about #60. It would break the py3 compat if pulled today due to some issues with the libs HTMLParser and markupbase.

Loading

@sloria
Copy link
Contributor Author

@sloria sloria commented Jul 12, 2014

@Hernrup If the Py3 builtin HTMLParser and the Py2 backport are fully compatibile, you could add a conditional dependency for Python2 in setup.py.

# setup.py
#...
if int(sys.version)[0] == 2:
    install_requires.append('HTMLParser')

Loading

import urllib
urljoin = urljoin
urlparse = urlparse
urlunparse = urlunparse
Copy link
Member

@d0ugal d0ugal Jul 22, 2014

Choose a reason for hiding this comment

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

I don't think the properties that are not being renamed should be listed like this.

Loading

Copy link
Contributor

@tomchristie tomchristie Jul 22, 2014

Choose a reason for hiding this comment

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

Agreed.

Loading

@sloria
Copy link
Contributor Author

@sloria sloria commented Jul 27, 2014

This PR is updated with modifications from @d0ugal's code review.

The Travis build is failing, but it seems to be failing on tests that are also failing on master.

Please let me know if any further modifications need to be made in order to merge this.

Loading

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Jul 28, 2014

You are right, there are four failing tests in the current master. I had hoped to solve them last week with help from @tomchristie at EuroPython but didn't manage.

Otherwise I think this change looks good.

Loading

@Keats
Copy link

@Keats Keats commented Aug 6, 2014

Any help needed to get that merged?

Loading

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Aug 7, 2014

I resolved the failing tests in master yesterday, it looks like that or another change means this now doesn't merge cleanly. So I think this just needs to be rebased and then i think it's good.

Loading

@Keats
Copy link

@Keats Keats commented Aug 8, 2014

ok, @sloria let me know if you don't have time, I can do it

Loading

@sloria
Copy link
Contributor Author

@sloria sloria commented Aug 8, 2014

Thanks @d0ugal for fixing those tests. I will get this updated with master later today.

Loading

@sloria
Copy link
Contributor Author

@sloria sloria commented Aug 10, 2014

OK, I've rebased on top of master. Ready for merge. Let me know if any other changes are necessary.

Loading

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Aug 10, 2014

Awesome! I'll get back to this during the week.

Loading

@@ -180,7 +181,7 @@ def build(config, live_server=False):
Perform a full site build.
"""
if not live_server:
print "Building documentation to directory: %s" % config['site_dir']
print("Building documentation to directory: %s" % config['site_dir'])
Copy link
Member

@d0ugal d0ugal Aug 10, 2014

Choose a reason for hiding this comment

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

Sorry, I missed this one before. We need a future import for print_function in this file.

Loading

@d0ugal
Copy link
Member

@d0ugal d0ugal commented Aug 10, 2014

Managed to find time to look through this today, I think this looks great! I spotted one file that is missing the print_function import but otherwise I'm happy to merge it in. Really pleased to see Python 3 support 😄

Loading

@sloria
Copy link
Contributor Author

@sloria sloria commented Aug 10, 2014

Added the missing import. Should be good to merge. Thanks for reviewing this.

Loading

d0ugal added a commit that referenced this issue Aug 11, 2014
Added support for Python 3.3 and 3.4.
@d0ugal d0ugal merged commit 189625c into mkdocs:master Aug 11, 2014
1 check passed
Loading
@d0ugal
Copy link
Member

@d0ugal d0ugal commented Aug 11, 2014

Thanks!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants