Skip to content

Conversation

andreif
Copy link

@andreif andreif commented Oct 19, 2015

...use enum instead of int in field value; raise validation error when enum is not found by value or name (and support digit str), etc

WARNING: Inconsistent field value atm -- enum for Django 1.8 and int for other versions when model is initialized

enum instead of int in field value; raise 
validation error when enum is not found by value 
or name (and support digit str), etc

WARNING: Inconsistent field value atm -- enum for 
Django 1.8 and int for other versions when model 
is initialized
.travis.yml Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

i think it would be totally fine to drop django 1.5 and django 1.6

Copy link
Author

Choose a reason for hiding this comment

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

We have some projects still running on Django 1.1 😨 Not sure where the border is

@fcurella
Copy link
Owner

I'm not entirely convinced we should worry that much about backward-compatibility -- as long as we version correctly (ie: increment the Major version) and provide a documented upgrade path

Copy link
Owner

Choose a reason for hiding this comment

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

I would keep the env vars specified as they currently are, just in case there's going to be a 1.9.99

Copy link
Author

Choose a reason for hiding this comment

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

We can always pad another 9 😺

Copy link
Owner

Choose a reason for hiding this comment

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

Does the DJANGO="Django>=1.8,<1.9" have any drawbacks? I'd rather have something consistently reliable, instead of a possible time bomb to patch in the future

Copy link
Author

Choose a reason for hiding this comment

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

It's harder to read and exclude. Will the shorter version fail in some cases?

Copy link
Owner

Choose a reason for hiding this comment

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

A correct statement is that the shorter version it will be incorrect in some edge cases, so it's really a perfectionist issue. I'm fine either way, I don't want this to become a distracting bikeshed

@fcurella
Copy link
Owner

enum for Django 1.8 and int for other versions

I can't find this in the diff. Can you point me to the right line?

@andreif
Copy link
Author

andreif commented Oct 19, 2015

Here you go https://github.com/fcurella/django-enumfield/pull/2/files#diff-174b02e9ae9348ecc51bb69323658307R48

Just to be clear, the issue is the result of my changes

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure why this include is needed. Isn't this ocmbo already covered by python * env?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, annoying thing, but it's needed due to allow_failures

Copy link
Owner

Choose a reason for hiding this comment

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

oh i see

@andreif
Copy link
Author

andreif commented Oct 20, 2015

I've tried SubfieldBase and according to the docs it should have helped as far as I understand, but it did not. I am putting debugging on hold atm and will come back to it when I have more time.

@andreif
Copy link
Author

andreif commented Oct 20, 2015

We are discussing it internally and looking into dropping support for Django 1.6-1.7 to avoid inconsistency and therefore making major release. What do you think about it?

@fcurella
Copy link
Owner

Django 1.7 is still officially supported by the Django project, I would advise to continue supporting it until 1.9 is out of beta. I think Django 1.6 can be safely dropped as it's unsupported now.

Copy link
Owner

Choose a reason for hiding this comment

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

to_python needs to handle the following cases:

  • value is None
  • value is a string
  • value is an enum

ref https://docs.djangoproject.com/en/1.8/howto/custom-model-fields/#converting-values-to-python-objects

Copy link
Author

Choose a reason for hiding this comment

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

enum.get does it

@fcurella
Copy link
Owner

It took some work to track it down, but I think I got IntEnum to work: https://travis-ci.org/5monkeys/django-enumfield/builds/86424033 5monkeys#28

@andreif
Copy link
Author

andreif commented Oct 20, 2015

Awesome!

@fcurella
Copy link
Owner

Looking good :)

I would add a Changelog (either its own file or a section in the readme) listing the breaking changes:

  • django_enumfield.enum.Enum is now a subclass of the native IntEnum
  • _transitions is now called __transitions__
  • _labels is now called __labels__

fcurella added a commit that referenced this pull request Oct 23, 2015
Change labels and transitions attr name, etc
@fcurella fcurella merged commit ea75df6 into fcurella:enum34 Oct 23, 2015
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.

2 participants