Add Python 3.7 support by rpkilby · Pull Request #6141 · encode/django-rest-framework (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
Codecov Report
Merging #6141 into master will increase coverage by
<.01%.
The diff coverage is100%.
@@ 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
| - { python: "2.7", env: DJANGO=2.1 } |
|---|
| - { python: "3.4", env: DJANGO=master } |
| - { python: "3.4", env: DJANGO=2.1 } |
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?
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.
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.
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
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(""" |
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 ☕️)
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.
Ryan P Kilby added 2 commits
Codecov Report
Merging #6141 into master will increase coverage by
<.01%.
The diff coverage is100%.
@@ 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.
Add Python 3.7 to CI Add Python 3.7 support
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request