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

Allowing BigIntegerField in taggedItems #693

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sowdust
Copy link

@sowdust sowdust commented Sep 30, 2020

Allows Taggable Items to have BigIntegerField as primary key. See #692

@@ -157,7 +157,7 @@ def tags_for(cls, model, instance=None, **extra_filters):


class GenericTaggedItemBase(CommonGenericTaggedItemBase):
object_id = models.IntegerField(verbose_name=_("object ID"), db_index=True)
object_id = models.BigIntegerField(verbose_name=_("object ID"), db_index=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will break backward compatibility

Copy link
Author

Choose a reason for hiding this comment

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

I see no other way to fix this issue : (

Copy link

Choose a reason for hiding this comment

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

They could do a version check of Django before migrating.
And use:

In [1]: import django
In [2]: django.VERSION
Out [2]: (3, 1, 1, 'final', 0)

Copy link
Contributor

Choose a reason for hiding this comment

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

probably moving to abstract base model is the best futuristic approach

@rtpg
Copy link
Contributor

rtpg commented Apr 14, 2021

This change as-is would break backwards compatibility with everyone using this base. After triaging a hundred+ issues, I think a big thing is the custom tag documentation. I'm going to work on improving it so that there could be a good answer here. But this as-is won't work, unfortunately

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.

4 participants