Alternate approach to enhancing Python 3 support #2918

gthank · 2017-05-30T23:37:59Z

The same basic work, but ordered differently and tested more incrementally.

…d commit we used to be on.

BEFORE: We were using a pinned commit where that functionality was added.

…ng it in.

Depend on futurize.

The futurize script made sure we were using the Unicode version of StringIO, so we needed to actually coerce incoming bytes to Unicode. This is a slight behavior change on Python 2, but I believe it to be an improvement because it makes another codepath Unicode-clean.

….communicate.

…s part of the test environment.

…qual instead.

…llision.

ericholscher · 2017-06-02T16:42:29Z

.travis.yml

@@ -1,9 +1,11 @@
 language: python
 python:
 - 2.7
+ - 3.6


I believe TOX handles the Python env, so having this both places means we run all of the tests 2x. I think we can remove this.

Or, we can remove the 2.7, and run them all in 3.6 -- I see travis fails with "no 3.6 found" when running under 2.7.

Yeah, I added 3.6 to get around the weird error in 2.7, but that exploded the build matrix, so I tried to add the tox-travis plugin to make the integration smarter, but never got it properly configured. If 2.7 is working under 3.6, I think the right play is to pull out the tox-travis plugin and just tell travis to use 3.6

ericholscher · 2017-06-02T16:44:14Z

Any thoughts on the lint error? I'm guessing it's a version mismatch between prospector and one of the other linting tools. We've run into that before.

Otherwise, this is amazing! I'll try and take a look and get it merged next week. ❤️

gthank · 2017-06-05T15:24:41Z

RE: the lint error -> It looks like pydocstyle added an option to check_source a few months back and prospector isn't passing it yet.

agjohnson · 2017-06-08T17:05:49Z

Dropping prospector back a minor release fixes this problem

agjohnson

Added mostly code cleanup feedback. I think to get this merged before it rots, lets mark the tests failing in py3 as expected failure for py3 environments and open a ticket to address resolving those issues. We'll stick with defaulting to py2 for now for tests/etc. Does this sound reasonable?

agjohnson · 2017-06-08T17:17:22Z

.travis.yml

@@ -1,9 +1,10 @@
 language: python
 python:
- - 2.7
+ - 3.6


Perhaps default to 2.7 for now

For some reason, Travis fails and says it can't find the Python 3.6 interpreter when we do it on 2.7. The 2.7 tests seem to work fine when we tell Travis to use 3.6. Apparently just some quirk with the Travis and tox interactions.

Huh, strange. Perhaps this is another motivator for switching to our newer standard tox.ini/travis.yml config, like: https://github.com/rtfd/sphinx_rtd_theme

If this is working as expected, probably no strong reason to switch just yet though

agjohnson · 2017-06-08T17:22:18Z

readthedocs/core/urls/single_version.py

@@ -8,6 +9,7 @@

 from readthedocs.constants import pattern_opts
 from readthedocs.core.views import serve
+from functools import reduce


import order is wrong here. should be pep8 standard of: builtins, third party, then local imports

agjohnson · 2017-06-08T17:22:31Z

readthedocs/core/urls/subdomain.py

 from operator import add

 from django.conf.urls import url, patterns
 from django.conf import settings
 from django.conf.urls.static import static

 from readthedocs.constants import pattern_opts
+from functools import reduce


import order off here too

agjohnson · 2017-06-08T17:23:19Z

readthedocs/doc_builder/config.py

 from readthedocs_build.config import (ConfigError, BuildConfig, InvalidConfig,
                                      load as load_config)


 from .constants import BUILD_IMAGES, DOCKER_IMAGE
+from six.moves import filter


import order off here as well

agjohnson · 2017-06-08T17:24:42Z

readthedocs/donate/models.py

@@ -16,8 +22,10 @@
 )
 from readthedocs.projects.models import Project
 from readthedocs.projects.constants import PROGRAMMING_LANGUAGES
+import six


import order here as well

agjohnson · 2017-06-08T17:50:27Z

readthedocs/vcs_support/backends/git.py

-    from io import StringIO
-else:
-    from StringIO import StringIO
+from io import StringIO


import order here

agjohnson · 2017-06-08T17:50:44Z

readthedocs/vcs_support/backends/git.py

@@ -113,10 +114,13 @@ def parse_tags(self, data):
        hash as identifier.
        """
        # parse the lines into a list of tuples (commit-hash, tag ref name)
+        # StringIO below is expecting Unicode data, so ensure that it gets it.
+        if not isinstance(data, str):
+            data = str(data)


str -> six.text_type again?

See notes about svn.py.

agjohnson · 2017-06-08T17:51:22Z

readthedocs/vcs_support/backends/git.py

@@ -147,9 +151,12 @@ def parse_branches(self, data):
              origin/release/2.1.0
        """
        clean_branches = []
+        # StringIO below is expecting Unicode data, so ensure that it gets it.
+        if not isinstance(data, str):
+            data = str(data)


str -> six.text_type here too?

See notes about svn.py.

agjohnson · 2017-06-08T17:51:44Z

readthedocs/vcs_support/backends/svn.py

-
+        # StringIO below is expecting Unicode data, so ensure that it gets it.
+        if not isinstance(data, str):
+            data = str(data)


str -> six.text_type here as well?

+from future import standard_library +standard_library.install_aliases()

basically makes str on Python 2 act like unicode and adds bytes as the equivalent to the original str.

Ah gotcha, missed that. These all make sense then.

agjohnson · 2017-06-08T17:53:37Z

requirements/pip.txt

 packaging==16.8

 # Commenting stuff
-git+https://github.com/zestedesavoir/django-cors-middleware.git@def9dc2f5e81eed53831bcda79ad1c127ecb3fe2
+django-cors-middleware==1.3.1


I'm not sure why this one was pinned to a commit. @ericholscher any background here?

As I recall, that commit was merging in a change from Eric that added some Django signals support, and now it's included in a public release.

Sounds right: zestedesavoir/django-cors-middleware@466d378

ericholscher · 2017-06-08T23:43:46Z

@gthank If you pull in this patch to fix linting (#2932, or just rebase it) and mark the failures as expected in py3, I'll merge this, so that it doesn't rot, and we don't have any regressions. Then we can work forward from there.

agjohnson · 2017-06-09T00:50:29Z

@ericholscher Could you first drop in on #2918 (comment), it's not clear why code was removed there, and that is also blocking review.

ericholscher · 2017-06-09T15:42:52Z

So close :)

ericholscher · 2017-06-09T15:43:21Z

Since this is only failing at linting, I'll do some local testing to confirm nothing is horribly broken, and it looks close! 🎆

agjohnson · 2017-06-09T15:51:05Z

@ericholscher sounds like this is close, we are discussing on IRC. probably no need to jump in. The plan for now is to get python3 tests working, which involve upgrading some of the lint/testing deps. I'm addressing that in #2863 and the merge will be cherrypicked here, so conflicts will be low. I'm less worried about rot as that PR and the restapi PR will be the only two conflicting commits and we can probably merge this all in early next week.

ericholscher · 2017-06-09T16:02:20Z

I think there will be manual testing required before we deploy these changes to prod, which is what I'm referring to. I'm happy to do it after we merge it.

…P 440.

ericholscher · 2017-06-09T17:50:48Z

Ugh @ restapi conflicts :( @agjohnson

agjohnson · 2017-06-10T01:19:56Z

Yup, this was discussed already and we anticipated having to rebase some work for this and fixing testing.

agjohnson · 2017-06-13T20:43:24Z

Okay, wrapped up #2863. I'm going to try to rebase this on a separate branch and see what happens.

agjohnson · 2017-06-14T16:40:54Z

Okay, we're all set here. I merged everything together in another PR, so I'll just close this PR and merge that branch into master.

I'm super excited to get going with python 3. Thanks a ton for putting in the work on this one, @gthank!

gthank · 2017-06-14T16:44:07Z

I'm excited, too! Thanks for letting me chip in, everybody.

gthank added 16 commits May 30, 2017 16:45

Add Python 3.6 to tox.ini

3b0c307

Update version of slumber.

cf7c021

Remove dependency on Distutils2; it's obsolete.

f2f0a88

Upgrade dnspython dependency.

ee016b2

Update haystack(ish) dependencies.

1ea4274

Update to latest django-celery.

e26b059

Upgrade to newer pytest-django for better Python3.

be9f1ed

Upgrade django-allauth to public release containing change from pinne…

09150cc

…d commit we used to be on.

Use modern Unipath.

96c2f6d

Use release of django-cors-middleware w/ signal support.

ff6813b

BEFORE: We were using a pinned commit where that functionality was added.

Explicitly depend on six, rather than waiting for a dependency to bri…

eee0978

…ng it in.

Make a pass w/ python-modernize.

f201d15

Make a pass with futurize on stage 2.

64116bc

Depend on futurize.

Apply Eric's Unicode version-parsing fix.

633877b

Apply Eric's fix for Unicode in auth.

87af24f

agjohnson added the PR: ready for review label May 30, 2017

Clean up bytes/unicode distinction when mocking results of subprocess…

1c22682

….communicate.

agjohnson added PR: work in progress Pull request is not ready for full review and removed PR: ready for review labels May 31, 2017

gthank added 10 commits May 31, 2017 15:55

Explicitly mock readthedocs.doc_builder.environments.api_v2.command a…

496b9c9

…s part of the test environment.

Use string.ascii_lowercase instead of string.lowercase when uniquifying.

7fa4239

Remove stray trailing comma in imports; that's an error in Python 3.

5ca33e6

Use assertTrue instead of assertTrueok_

806b065

Ignore tags file from exuberant ctags.

a4a1bab

unittest.assertItemsEqual went away in Python 3, use six.assertCountE…

0bd9294

…qual instead.

Undo a .next() -> next() from futurize; it was an unfortunate name co…

0b70237

…llision.

Don't try to use BaseException.message

cbcdbcb

Fix file pathing error caused by differences between 2 and 3.

2401401

Fix bytes/str in test_version_commit_name, and be explicit.

959cebd

gthank added 4 commits June 1, 2017 14:51

Tell Travis we want python 3.6 too

f351f85

Make Python 2.7 and Python 3.6 separate Travis builds.

76e1117

More Travis-CI tweakery.

e46be85

Fix wonky syntax.

0d73ebe

ericholscher reviewed Jun 2, 2017

View reviewed changes

Try to simplify the tox/travis interactions.

1d055ef

agjohnson requested changes Jun 8, 2017

View reviewed changes

gthank added 2 commits June 8, 2017 15:43

Clean up some import order issues.

6e8027b

Parse the URL using the right name.

f972a55

agjohnson and others added 3 commits June 9, 2017 10:52

Pin prospector to previous minor release

717da9a

Flag three tests as known failues under Py3.

ea3096f

Remove unnecessary 'cast' to unicode string.

dd6b557

Make version-parsing code use only ASCII chars, in accordance with PE…

027f734

…P 440.

Merge branch 'master' into master

3d5191b

agjohnson closed this Jun 14, 2017

agjohnson removed the PR: work in progress Pull request is not ready for full review label Jun 14, 2017

Alternate approach to enhancing Python 3 support #2918

Alternate approach to enhancing Python 3 support #2918

Conversation

gthank commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher commented Jun 2, 2017 • edited Loading

gthank commented Jun 5, 2017

agjohnson commented Jun 8, 2017

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher commented Jun 8, 2017 • edited Loading

agjohnson commented Jun 9, 2017

ericholscher commented Jun 9, 2017

ericholscher commented Jun 9, 2017

agjohnson commented Jun 9, 2017 • edited Loading

ericholscher commented Jun 9, 2017

ericholscher commented Jun 9, 2017

agjohnson commented Jun 10, 2017

agjohnson commented Jun 13, 2017

agjohnson commented Jun 14, 2017

gthank commented Jun 14, 2017

ericholscher commented Jun 2, 2017 •

edited

Loading

ericholscher commented Jun 8, 2017 •

edited

Loading

agjohnson commented Jun 9, 2017 •

edited

Loading