Skip to content

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

Merged
merged 2 commits into from
Sep 11, 2018
Merged

Add Python 3.7 support #6141

merged 2 commits into from
Sep 11, 2018

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Aug 24, 2018

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-io
Copy link

codecov-io commented Aug 24, 2018

Codecov Report

Merging #6141 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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 }

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Collaborator

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

Copy link
Member Author

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("""
Copy link
Collaborator

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 ☕️)

Copy link
Member Author

@rpkilby rpkilby Aug 29, 2018

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-io
Copy link

codecov-io commented Sep 11, 2018

Codecov Report

Merging #6141 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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

@rpkilby
Copy link
Member Author

rpkilby commented Sep 11, 2018

Going ahead and merging - the travis reorg has been pulled into a separate branch.

@rpkilby rpkilby changed the title Add Python 3.7 to CI Add Python 3.7 support Sep 11, 2018
@rpkilby rpkilby merged commit 7f77340 into encode:master Sep 11, 2018
@rpkilby rpkilby deleted the python37 branch September 11, 2018 04:44
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
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.

5 participants