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

#6 support django 2 #48

Merged
merged 5 commits into from
Nov 13, 2018
Merged

#6 support django 2 #48

merged 5 commits into from
Nov 13, 2018

Conversation

codekiln
Copy link
Contributor

@codekiln codekiln commented Nov 11, 2018

Dependency: PR #46 remove support for python 2. That should be merged first, removing commits from this PR.

Adds support to Django 2.0 for #6

* fixed a `queue` import error with multiprocessing_utils.py
* Added Django 2.0 and 2.1 to test matrix
* Added models.CASCADE to every foreign key except one:
  Index.active_version. Generated migration for that one to set it to
  null upon delete instead.
* use utf-8 encoding for generating hash, but save bytestring to db
  instead, so as to avoid saving `b'{json here}'` into the `json` field
  of IndexVersion in Django 2; see
  https://docs.djangoproject.com/en/2.1/releases/2.0/#removed-support-for-bytestrings-in-some-places
  for more info.

#6 support django 2
@codecov
Copy link

codecov bot commented Nov 11, 2018

Codecov Report

Merging #48 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #48   +/-   ##
=======================================
  Coverage   70.96%   70.96%           
=======================================
  Files          18       18           
  Lines         847      847           
  Branches      132      132           
=======================================
  Hits          601      601           
  Misses        192      192           
  Partials       54       54
Impacted Files Coverage Δ
django_elastic_migrations/utils/es_utils.py 100% <100%> (ø) ⬆️
..._elastic_migrations/utils/multiprocessing_utils.py 66.67% <100%> (ø) ⬆️

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 a247cf4...86d5729. Read the comment docs.

md5_hash = hashlib.md5()
md5_hash.update(json_str)
md5_hash.update(json_str.encode('utf-8'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Django 2.0 release notes, they mention they "Removed support for bytestrings in some places". One of those places is saving text to a standard TextField, where starting in 2.0 if you save a bytestring it looks like b'my bytestring' in the database; you need to supply it a normal str to get the expected behavior.

At the same time, md5_hash requires a bytestring, so this moves the .encode('utf-8') to just that function, but returns the str that comes out of json.dumps.

migrations.AlterField(
model_name='index',
name='active_version',
field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to='django_elastic_migrations.IndexVersion'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Django 2.0 requires explicit on_delete handlers for foreign keys. I've used the default, models.CASCADE, in all places except one: Index.active_version, which should just be set to null if the IndexVersion is deleted. This migration captures that change.

@codekiln codekiln merged commit 3cfa2fd into master Nov 13, 2018
@codekiln codekiln deleted the #6_support_django_2 branch November 13, 2018 16:04
@codekiln
Copy link
Contributor Author

closes #44 add django 1.10 and 1.11 to test matrix

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.

3 participants