Subclass TimeflakePrimaryKeyBinary from UUIDfield
sebst opened this issue · 6 comments
UUIDField is build into Django and should therefore be preferred as it will likely increase compat with other third party libraries such as DRF.
See previous issue #8 .
However, doing a subclassing now would likely result in migration incompatible changes (as the db type may change and foreign key relations could point to this). This would justify a Major Version increase according to SemVer even if only the django extension would be affected.
So, imho, there are two options:
- Do the subclass now and increase to 1.0.0
- Remove the django extension and make a new Python package like django-timeflake.
What do you think, @anthonynsimon ?
I think subclassing and doing a major version bump would be best. My main worry would be if many people already rely on this, it might lead to a painful migration (UUIDField might not map 1:1 with the current types used in DBs).
For example, while in Postgres Timeflake uses the native uuid type, in MySQL and in SQLite it uses binary types. We'd need to ensure that the UUIDField built into Django translates properly in those databases too.
Ref in Timeflake:
https://github.com/anthonynsimon/timeflake/blob/master/timeflake/extensions/django/__init__.py#L42
In Django's codebase:
You're right, the concern about painful migrations is exactly what would justify the major version increase.
A migration path for the timeflake package might be:
- Mark the current (pre-PR)
TimeflakeBinary
deprecated, support the0.x.x
branch for some time. - Rename
TimeflakeBinary
in the PR toTimeflakeUUID
and keep theTimeflakeBinary
, but raise a deprecation warning on usage. 2.x.x
should then remove the django extension entirely and provide a separatetimeflake-django
package.
Another option would could be (because we already subclassed Timeflake
from UUID
) to just keep things as they are and just update the documentation, such that users are advised to use a primary key like this
from timeflake import random
class MyModel(models.Model):
id = models.UUIDField(primary_key=True, editable=False, default=random)
Hey thanks for the suggestions!
Maybe it makes sense to instead provide different classes for the different encodings? That way we keep full backwards compatibility (no changes required for existing users), while better supporting the ecosystem (DRF, admin extensions, and so on).
For example, we could introduce two new fields: TimeflakeBase62Field
, and TimeflakeUUIDField
, while keeping TimeflakeBinaryField
.
That way users could pick the encoding that best fits their needs, given the pros and cons of each (TimeflakeUUIDField would work out of the box with stuff that already handles standard UUID fields, while TimeflakeBinaryField not).
As a plus, it would also be consistent with the peewee integration here: https://github.com/anthonynsimon/timeflake/blob/master/timeflake/extensions/peewee/__init__.py
What do you think?
I will also alias the existing TimeflakeBinary
with TimeflakeBinaryField
to make this consistent with Django :)
Yes, that sounds great
Hey do you need any assistance pushing this through? Looks like there's a PR out with the implementation already.