Skip to content
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

Fixing compatibility issues with Python3 #17

Merged
merged 1 commit into from
Jul 12, 2018
Merged

Fixing compatibility issues with Python3 #17

merged 1 commit into from
Jul 12, 2018

Conversation

Ivanca
Copy link

@Ivanca Ivanca commented Jul 9, 2018

Background: An effort is being made to move everything from Python 2.7 to 3, including making this repository compatible with both as specified on this JIRA issue: https://openedx.atlassian.net/projects/INCR/issues/INCR-18?filter=allopenissues and these changes aim to do that.

Studio Updates: None.

LMS Updates: None

Testing: Same ones as before (and have been updated to be compatible with both as well), passing.

@openedx-webhooks
Copy link

Thanks for the pull request, @Ivanca! I've created OSPR-2511 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jul 9, 2018
@codecov
Copy link

codecov bot commented Jul 9, 2018

Codecov Report

Merging #17 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   93.47%   93.62%   +0.15%     
==========================================
  Files          12       12              
  Lines         628      643      +15     
  Branches       34       44      +10     
==========================================
+ Hits          587      602      +15     
  Misses         34       34              
  Partials        7        7
Impacted Files Coverage Δ
config_models/tests/test_decorators.py 95% <100%> (+0.55%) ⬆️
...onfig_models/management/commands/populate_model.py 91.3% <100%> (+0.39%) ⬆️
config_models/models.py 96.66% <100%> (+0.03%) ⬆️
config_models/tests/test_model_deserialization.py 99.14% <100%> (+0.05%) ⬆️
config_models/tests/test_config_models.py 98.93% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d663fce...6fc59c6. Read the comment docs.

@Ivanca
Copy link
Author

Ivanca commented Jul 9, 2018

I don't fully understand what the "codevob" is asking for here; more clarity in the changes that need to be made (if any) would be appreciated.

Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You added a few lines of code to the test that are only executed under Python 3, so codecov is complaining that the percentage of the file executed during the Python-2-only test suite decreased slightly. Try adding testing for Python 3.5 and/or 3.6 in tox.ini and .travis.yml to fix that. Feel free to remove tests for Django versions below 1.11 while you're at it. Links to the relevant tox and Travis documentation are below, let me know if you want help figuring it out:

@@ -9,7 +9,7 @@
from django.utils.translation import ugettext_lazy as _

from rest_framework.utils import model_meta

import six
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see six among the dependencies in setup.py yet. Maybe just use the version bundled with django.utils as you used in the file below?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah; fixed now on last commit (imports from django.utils)

@jmbowman
Copy link
Contributor

The errors under Python 3.6 are due to the futures dependency in requirements/test.txt; it's a backport of native Python 3 functionality and hence hasn't been packaged for installation in a Python 3 environment. You should be able to fix that by adding futures ; python_version == "2.7" to requirements/test.in (a similar fix was made in bok-choy). The need to work around this manually is a bug in pip-tools.

@Ivanca
Copy link
Author

Ivanca commented Jul 11, 2018

Finally all checks passing now 👍 @jmbowman
Travis CI is testing on both 2.7 and 3.6

Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Could you fix one more problem while you're in here? The last merge to master was marked as a failure because all of the Travis workers tried deploying to PyPI, and only the one that finished first succeeded. You should only need to add appropriate python and condition entries to the on block in .travis.yml, like the ones here.

@@ -62,5 +62,6 @@ def get_version(*file_paths):
'Natural Language :: English',
'Programming Language :: Python :: 2',
'Programming Language :: Python :: 2.7',
'Programming Language :: Python :: 3',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably add an entry for 3.6 here also.

@Ivanca
Copy link
Author

Ivanca commented Jul 12, 2018

done

Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! Please squash the commits, then I'll go ahead and merge this and prepare a new release.

@Ivanca
Copy link
Author

Ivanca commented Jul 12, 2018

Commits squashed.

@jmbowman jmbowman merged commit 4906a27 into openedx:master Jul 12, 2018
@openedx-webhooks
Copy link

@Ivanca 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants