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

Copying Dirty Fields #10

Closed
wants to merge 2 commits into from
Closed

Copying Dirty Fields #10

wants to merge 2 commits into from

Conversation

Xowap
Copy link

@Xowap Xowap commented Mar 14, 2014

The dirty fields mixin works fine for simple types like string, booleans or integers, however it fails when it comes to more complex objects like geometries or JSON fields.

Take the following model

class Test(models.Model):
    anchor = PointField()

Then do something like

t = Test.objects.get(id=1)
t.anchor.x = 42
t.get_dirty_fields()

In the current version, this returns an empty array. However with the attached patch, it will indeed return "anchor" as a changed field, with the correct previous value.

Technical choices for this patch are explained in details in efd0286.

There is also a bit of PEP8 formatting in 6b4dcee.

Otherwise, the behaviour becomes bogus when dealing with objects more complex
than simple types, like GEOS geometries.

Won't the copy take more memory? Well, very few, because it is a shallow copy,
and thus all objects referenced by the root object stay the same.

Shouldn't this be optional? For all the use cases that worked currently, it
doesn't change anything (a copy of a bool or of a string will use the same
amount of memory than before), and for other cases it is the only way to solve a
critical bug. So there is no point making this optional.
@dacresni
Copy link

dacresni commented May 6, 2014

wow, please send this patch to me

@Xowap
Copy link
Author

Xowap commented May 6, 2014

what do you mean?

@dacresni
Copy link

dacresni commented May 8, 2014

I actually forked this because i didn't know this existed https://github.com/django/django/blob/1.5.7/django/forms/forms.py#L347 i can't accept a pull request directed at a different repo. I'm not sure if this repo is still relevant. knowing that django forms have this capability already, is having it on models still necessary?

@Xowap
Copy link
Author

Xowap commented May 8, 2014

Yup it is needed for me, as I use TastyPie and almost no forms. Furthermore this is a warranty of getting stuff right at the smallest possible granularity in Django.

@Xowap
Copy link
Author

Xowap commented May 8, 2014

PR sent

@romgar
Copy link
Owner

romgar commented Apr 8, 2015

Hi @Xowap , as there are some issues with other types of fields, I will think about a global solution and keep you up-to-date.

@romgar
Copy link
Owner

romgar commented Jun 12, 2015

PR #41 will fix this. Thanks for your help @Xowap

@romgar romgar closed this Jun 12, 2015
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.

None yet

3 participants