-
-
Notifications
You must be signed in to change notification settings - Fork 7k
Add Python 3.7 support #6141
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
Add Python 3.7 support #6141
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6141 +/- ##
==========================================
+ Coverage 96.18% 96.18% +<.01%
==========================================
Files 128 128
Lines 17624 17625 +1
Branches 1459 1459
==========================================
+ Hits 16951 16952 +1
Misses 465 465
Partials 208 208 |
.travis.yml
Outdated
- { python: "2.7", env: DJANGO=2.1 } | ||
- { python: "3.4", env: DJANGO=master } | ||
- { python: "3.4", env: DJANGO=2.1 } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm finding the change in .travis.yml
hard to follow with these other bits moved around. Is it possible to add 3.7 to the matrix in a more minimal way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you looked at the first two commits individually? The matrix reorganization and addition of 3.7 are separate. If preferred, I can split the matrix reorg into another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do want to reorganize the matrix though, as it is difficult to follow the combination of include, exclude, and matrix expansion. The single include section is a little more straightforward to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't just adding to the python
field work...?
python:
- "3.4"
- "3.5"
- "3.6"
- "3.7-dev"
A la what we did on django-filter
If preferred, I can split the matrix reorg into another PR.
That might be worth it — mostly because of the fix in test_model_serializer.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.7-dev is very outdated. In order to use the official release, you need to install with Ubuntu xenial+sudo: true
. The workaround is covered in travis-ci/travis-ci#9815
@@ -381,6 +382,10 @@ class Meta: | |||
TestSerializer(): | |||
id = IntegerField(label='ID', read_only=True) | |||
duration_field = DurationField(max_value=datetime.timedelta(3), min_value=datetime.timedelta(1)) | |||
""") if sys.version_info < (3, 7) else dedent(""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happening here? (I need a ☕️)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python 3.7 changed the timedelta
repr to include the argument names. So, previously this was timedelta(3)
, now it's timedelta(days=3)
. This check is necessary for compatibility. IDK if there is a better way of testing the python version here.
Codecov Report
@@ Coverage Diff @@
## master #6141 +/- ##
==========================================
+ Coverage 96.18% 96.18% +<.01%
==========================================
Files 128 128
Lines 17629 17630 +1
Branches 1458 1458
==========================================
+ Hits 16956 16957 +1
Misses 465 465
Partials 208 208 |
Going ahead and merging - the travis reorg has been pulled into a separate branch. |
Attempt at adding Python 3.7, which requires switching to Xenial. I've also updated the Travix matrix to be an explicit include list, as opposed to a mix of both include and exclude.