-
Notifications
You must be signed in to change notification settings - Fork 647
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
Bump Django to 1.11.17 and add Python 3.7 in test matrix #4551
Bump Django to 1.11.17 and add Python 3.7 in test matrix #4551
Conversation
unclear to me why tests are failing:
|
@indirectlylit it's because of |
Doing some work for the Iceqube CI, so we can have automated tests approve these changes... |
This PR is pending that I have credentials for PyPi to release another Iceqube... |
Hrm, sorry, I don't have creds for icequebe on pypi either. |
Iceqube 0.1b1 is released! |
128e71b
to
3d67508
Compare
Codecov Report
@@ Coverage Diff @@
## release-v0.11.x #4551 +/- ##
===================================================
+ Coverage 50.13% 51.92% +1.79%
===================================================
Files 866 739 -127
Lines 28715 24385 -4330
Branches 4154 3304 -850
===================================================
- Hits 14395 12661 -1734
+ Misses 13503 10959 -2544
+ Partials 817 765 -52
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## release-v0.11.x #4551 +/- ##
==================================================
Coverage ? 51.96%
==================================================
Files ? 739
Lines ? 24403
Branches ? 3308
==================================================
Hits ? 12682
Misses ? 10955
Partials ? 766 Continue to review full report at Codecov.
|
What what what. @lyw07 have you seen this before?
|
Ah yes. I just created a PR(#4678) targeting 0.9 to fix it. Will get the fix in to branches above 0.9 after the PR is merged. |
Thanks so much, that makes total sense -- sorry haven't familiarized myself with all the latest work of 2019 :) |
There are a handful of Python 3.7 related issues in tests that I will investigate, and the issue with the missing ARM build for Cryptography isn't really a blocker for fixing this. I'll try and dig into it and assess if its feasible to fix for 0.11.1 -- and if not, retargetting this PR is easy. |
6fda6b4
to
5dbc61b
Compare
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.
This is looking good, and I think would be good to include in 0.11.1 if at all possible.
@@ -32,9 +32,17 @@ matrix: | |||
- python: "3.6" | |||
env: | |||
- TOX_ENV=pythonbuild3.6 | |||
- python: 3.7 | |||
dist: xenial | |||
sudo: true |
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.
Is this a travis requirement to run with sudo true for Python3.7?
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.
Yes, it's because it obtains Python 3.7 from some addition source and installs it. We cannot specify "Python 3.7" without these extra options. It's been the case for quite some time now, but Travis seems to need some time for releasing new images.
kolibri/core/webpack/utils.py
Outdated
|
||
# Backwards compatibility: The reserved keyword 'async' was used before. | ||
# May be removed in 0.12+. | ||
is_async = kwargs.pop('async', is_async) |
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.
Good good - yeah, maybe add a deprecation warning if kwargs.pop('async')
returns something?
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.
Good point, let's have them visible! Fixed!
from django import template | ||
from kolibri.core.webpack.utils import webpack_asset_render |
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 thought we had already linted every file? When we run CI linting, does it not run our import linting too?
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.
This may not be seen by a linting rule but is likely only caused by changes to our pre-commit hooks.
ad4f887
to
8d77ed9
Compare
8d77ed9
to
888ee85
Compare
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.
Mr. Travis will have the final say, but this gets my vote!
Two tiny linting errors on the Python3 lint. |
Ignore undefined linting of Py2 only globals in Py2 only code blocks.
Yup! CC: @radinamatic - we could need a bit of Python 3.7 testing before announcing the support. I'll have a look at building a Debian package that doesn't force any Python downgrades. |
Summary
This adds Python 3.7 compatibility, assuming that it's confirmed with by CI...
Reviewer guidance
Any discussion points about targeting this for 0.11.1?
References
#4548
Contributor Checklist
Reviewer Checklist
yarn
andpip
)