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

Subclass TimeflakePrimaryKeyBinary from UUIDfield #10

Open
sebst opened this issue Apr 19, 2021 · 6 comments
Open

Subclass TimeflakePrimaryKeyBinary from UUIDfield #10

sebst opened this issue Apr 19, 2021 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@sebst
Copy link
Contributor

sebst commented Apr 19, 2021

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 ?

@anthonynsimon
Copy link
Owner

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
Copy link
Contributor Author

sebst commented Apr 23, 2021

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)

@anthonynsimon
Copy link
Owner

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?

@anthonynsimon
Copy link
Owner

I will also alias the existing TimeflakeBinary with TimeflakeBinaryField to make this consistent with Django :)

@sebst
Copy link
Contributor Author

sebst commented May 15, 2021

Yes, that sounds great

@RobRoseKnows
Copy link

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

@anthonynsimon anthonynsimon added enhancement New feature or request help wanted Extra attention is needed labels Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants