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

Enum conversion inconsistencies #60

Closed
andrewdodd opened this issue Jun 23, 2016 · 4 comments
Closed

Enum conversion inconsistencies #60

andrewdodd opened this issue Jun 23, 2016 · 4 comments

Comments

@andrewdodd
Copy link

andrewdodd commented Jun 23, 2016

When upgrading from 0.7.4 to 0.8.0 I found that the behaviour of enum fields has changed if you are initialising them via their String value rather than by their enum type.

This behaviour was a side effect of the changes to remove the deprecated SubfieldBase (8260050).

I have had a short attempt at fixing this but haven't managed to find a way. (I'm thinking from my reading of the SubfieldBase related material that it might have something to do with replacing the __new__ method).

I have written a test to demonstrate the issue:

# -- encoding: UTF-8 --

import pytest

from .models import MyModel

try:
    from .enums import Color # Use the new location of Color enum
except ImportError:
    Color = MyModel.Color    # Attempt the 0.7.4 location of color enum

@pytest.mark.django_db
def test_fields_value_is_enum_when_unsaved():
    obj = MyModel(color='r')
    assert Color.RED == obj.color

@pytest.mark.django_db
def test_fields_value_is_enum_when_saved():
    obj = MyModel(color='r')
    obj.save()
    assert Color.RED == obj.color

@pytest.mark.django_db
def test_fields_value_is_enum_when_created():
    obj = MyModel.objects.create(color='r')
    assert Color.RED == obj.color

@pytest.mark.django_db
def test_fields_value_is_enum_when_retrieved():
    MyModel.objects.create(color='r')
    obj = MyModel.objects.first()
    assert Color.RED == obj.color

To see the failure copy this test into a file in the tests directory and run

> python manage.py test

To see it pass, checkout 0.7.4 and run it again!

@akx
Copy link
Contributor

akx commented Jun 27, 2016

Thanks for the bug report!

Sure enough, that violates the Principle of Least Astonishment, and is a change in behavior from 0.7.x.

I dug in a little, and created this Django ticket about this: https://code.djangoproject.com/ticket/26807

akx added a commit that referenced this issue Jun 27, 2016
Turns out the future-deprecated SubfieldBase class did more
than what was initially thought.  Since it is removed in
Django 1.10+, for clarity we no longer use it at all.
Instead we intern the behavior of its `Creator` descriptor
class as `CastOnAssignDescriptor`.

Fixes #60
Refs https://code.djangoproject.com/ticket/26807
Refs 8260050
akx added a commit that referenced this issue Jun 27, 2016
akx added a commit that referenced this issue Jun 27, 2016
Turns out the future-deprecated SubfieldBase class did more
than what was initially thought.  Since it is removed in
Django 1.10+, for clarity we no longer use it at all.
Instead we intern the behavior of its `Creator` descriptor
class as `CastOnAssignDescriptor`.

Fixes #60
Refs https://code.djangoproject.com/ticket/26807
Refs 8260050
akx added a commit that referenced this issue Jun 27, 2016
Turns out the future-deprecated SubfieldBase class did more
than what was initially thought.  Since it is removed in
Django 1.10+, for clarity we no longer use it at all.
Instead we intern the behavior of its `Creator` descriptor
class as `CastOnAssignDescriptor`.

Test cases and original bug report by @andrewdodd. Thanks!

Fixes #60
Refs https://code.djangoproject.com/ticket/26807
Refs 8260050
@akx akx closed this as completed in #61 Jun 27, 2016
@akx
Copy link
Contributor

akx commented Jun 27, 2016

@andrewdodd: This is fixed in 0.8.1

@andrewdodd
Copy link
Author

Holy moly that was quick!

I dug a little into this on the weekend and came up with the same idea... that one would have to re-implement the SubfieldBase/Creator code, which felt bad.

Thanks!

@akx
Copy link
Contributor

akx commented Jun 27, 2016

@andrewdodd Yeah, it felt strange, but looks like that's the way to go nevertheless -- and at the very least it fixes things for the time being.

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

No branches or pull requests

2 participants