Skip to content

Conversation

andreif
Copy link

@andreif andreif commented Nov 16, 2015

Related to 5monkeys#25

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 I agree on the ambiguity that this introduces. To quote the zen of Python "There should be one-- and preferably only one --obvious way to do it."

What bother me most is that having two ways to declare the default means I'll have to check both every time the field does not have a default declared.

Assume the following scenario: I've inherited a code-base, and I'm going through its models and I see:

from .enums import BeerStyle


class Beer(models.Model):
  style = enum.EnumField(BeerStyle)

Now I have to check 2 places to find out if there is a default: the field declaration above, and the enum declaration. This for every single time the EnumField is used with default.

Copy link
Author

Choose a reason for hiding this comment

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

Well, Enum itself has only one place to set default i.e. via __default__ attribute. Previously, one would need to override .default() method (still possible) and it was not obvious/expected that the first value is the default one. One could restore the old behaviour by subclassing Enum whenever it's needed:

from django_enumfield import enum

class FirstDefaultEnum(enum.Enum):
    def default(cls):
        return list(cls)[0]

EnumField on the other hand may be independent enough to override the default just if it's needed. For example, the same enum may be used in various places and who knows maybe someone will need to override the default just in one of them. I believe one expects this to be possible in a field.

Copy link
Owner

Choose a reason for hiding this comment

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

I dont see any mention of __default__ on the python docs (https://docs.python.org/3/library/enum.html)

I think we should keep our Enum subclass as close as possible to the original, without supercharging it further with custom methods or properties and add the features we need only to the field instead.

Copy link
Owner

Choose a reason for hiding this comment

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

I see the convenience of declaring __default__ just once, but I'm not sure that's an advantage in the long run. I'd rather be explicit than save some keystrokes.

Plus, I would make the argument that the default is a property of the field, and therefore belongs to the field declaration. Just in the same spirit as Field.choices works. The Enum is data, while default= is behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so one could define a custom field for certain enum in case it's used in many places. Sure, let me discuss it with my colleagues to see if everyone is happy about it and redo the commits 😄

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