-
Notifications
You must be signed in to change notification settings - Fork 18
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
Comments
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:
Another option would could be (because we already subclassed
|
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: 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 |
Yes, that sounds great |
Hey do you need any assistance pushing this through? Looks like there's a PR out with the implementation already. |
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:
What do you think, @anthonynsimon ?
The text was updated successfully, but these errors were encountered: