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

Add support for GenericFK fields #38

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aschriner
Copy link
Contributor

Added generator for GenericFK relations.
Overall method: find GenericFk fields from model._meta.virtual_fields,
add them to the list of fields to be processed, and remove the
constituent content_type and object_id fields from the processing list.
Then use the GenericFK generator to either select or create an
object to assign.

Addresses #32

Overall method: find GenericFk fields from model._meta.virtual_fields,
add them to the list of fields to be processed, and remove the
constituent content_type and object_id fields from the processing list.
Then use the GenericFK generator to either select or create an
object to assign.
@aschriner
Copy link
Contributor Author

Needs tests. Looks like you've got a big test suite - if you point me to where the tests should fit into the test suite I will write them.

@@ -230,6 +235,17 @@ def add_constraint(self, constraint):
'''
self.constraints.append(constraint)

def _normalize_genericfk_field(self, field):
Copy link
Owner

Choose a reason for hiding this comment

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

Does this method monkey patch the actual field object that comes from the model? I think it might be better to not do that and instead return a copy or a mock of the actual field. That way we don't create sideeffects for any other code that uses the model. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you are right. good catch - i will change that function to return a copy of the field object.

@gregmuellegger
Copy link
Owner

Soooo great! Thanks for this awesome pull request.
I would say the tests for GenericFKSelector go into autofixture_tests/tests/generator.py were as the tests for the changes on the AutoFixture class go into autofixture_tests/tests/base.py. But feel free to create a new file if you think this should go somewhere else.

Cheers! :)

_normalize_genericfk_field() now operates on a copy of the original field instead of the original field object
@@ -237,6 +254,9 @@ def get_generator(self, field):
specified field will be ignored (e.g. if no matching generator was
found).
'''
if isinstance(field, GenericForeignKey):
field = self._normalize_genericfk_field(field)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not return generator right here?

@AndreiPashkin
Copy link
Contributor

I think theres simpler solution exists, without monkeypatching etc.
What I dont understand is the Link class - how it works? I think it must be used for handling nested GFK relations. Greg, can you make a commit with dtailed comments for this class and its methods?

@gregmuellegger
Copy link
Owner

Gosh, that is code which is over 4 years old and that I wrote in the middle of the night at that time 😅

I just made an attempt to document but changed my mind and will try to make my best to refactor this one out and replace it with something that's easier to understand... I think that's easier and more future proof.

@AndreiPashkin
Copy link
Contributor

I think that Link must not know anything about nested relations and represent and generalize only one relation, with a parameters such as one/many, taget content-type, target object-id (i think its enough to cover all types of relations). Nested relations can be handled by some clever recursive function (beware of inifinite recursion!), which will be invoked during fixture creation process when AutoFixture finds any relation.

@m000
Copy link

m000 commented Sep 12, 2016

Are there plans to add this feature?

In my gfk branch, I've merged the PR and resolved the conflicts and a couple of minor issues.

@alb3rto269
Copy link

alb3rto269 commented Mar 23, 2018

Here is how I'm using AutoFixture in models with GenericForeignKey.
Feel free to use and/or modify it.

https://gist.github.com/alb3rto269/def529c94f47272bed8d2990c93d99f2

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.

5 participants