anthonynsimon/timeflake

Subclass TimeflakePrimaryKeyBinary from UUIDfield

sebst opened this issue · 6 comments

sebst commented

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:

  1. Do the subclass now and increase to 1.0.0
  2. 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:

https://github.com/django/django/blob/ca9872905559026af82000e46cde6f7dedc897b6/django/db/models/fields/__init__.py#L2420

sebst commented

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:

  1. Mark the current (pre-PR) TimeflakeBinary deprecated, support the 0.x.x branch for some time.
  2. Rename TimeflakeBinary in the PR to TimeflakeUUID and keep the TimeflakeBinary, but raise a deprecation warning on usage.
  3. 2.x.x should then remove the django extension entirely and provide a separate timeflake-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 :)

sebst commented

Yes, that sounds great

Hey do you need any assistance pushing this through? Looks like there's a PR out with the implementation already.