-
Notifications
You must be signed in to change notification settings - Fork 399
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
Make _setup_next_sequence
usable with DictFactory
subclasses
#1056
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for taking the time to contribute a PR, with tests. 😁
Sadly, the suggested approach introduces a difference in behavior between DictFactory
and the other factories.
DictFactory
sets a model
in its options, which results in all DictFactory
subclasses sharing the same base factory (and reference_counter
): DictFactory
. Hence, only the _setup_next_sequence()
from DictFactory
is executed, and it returns 0. That is undesirable, but that’s the current behavior.
With the suggested patch, each factory in the dict hierarchy can reset its sequence, so you can make a subclass of Dog
and have it define a _setup_next_sequence()
to start from 0. Whereas, for other factories, all subclasses use the same sequence as the top-most factory that defines a model in the inheritance. To illustrate, the following test does not pass:
def test_sequence_resets_on_subclass(self):
class Base42Factory(factory.Factory):
class Meta:
model = TestObject
@classmethod
def _setup_next_sequence(cls):
return 42
one = factory.Sequence(lambda n: f'{n}')
class Base9000Factory(Base42Factory):
@classmethod
def _setup_next_sequence(cls):
return 9000
base9000 = Base9000Factory.build()
self.assertEqual('9000', base9000.one) # Is '42'.
base42 = Base42Factory.build()
self.assertEqual('42', base42.one)
Possible ideas:
- follow the existing practice, maybe special-casing the
DictFactory
so that the reference_counter points to the top-most factory beneathDictFactory
in the inheritance. - allow resetting sequences for all factories.
I think the second would make for a nicer behavior, but that requires heavier changes to how the library works (and updating the docs to clearly define the behavior).
def test_descendants_share_counter(self): | ||
cat_result = self.Cat() | ||
dog_result = self.Dog() | ||
results = [cat_result['pet_id'], dog_result['pet_id']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this list is necessary? Why not directly assert the cat_result['pet_id']
and dog_result['pet_id']
?
_setup_next_sequence
usable with DictFactory
subclasses
Add new option class for
DictFactory
that checks for the implementation of_setup_next_sequence
to determine counter reference.Fixes #993