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

Introduce factory.declaration.Transformer #623

Merged

Conversation

francoisfreitag
Copy link
Member

Transforms a value using provided transform function. Values coming from the declaration and values overridden through keywords arguments are transformed before the generated object attribute is set.

Removes the need to save objects with a post generation hook twice to the database.

Facilitates overriding Django passwords when instantiating the factory.

Fixes #316
Fixes #366

@francoisfreitag
Copy link
Member Author

I believe it would be best to make a minor release first, to get the small fixes from the past weeks out to users. Perhaps the release should be a 2.13.0, since Python 3.4 and Django 2.0 compatibility was dropped.

Then, we can discuss integrating this work.

@MRigal
Copy link
Contributor

MRigal commented Nov 2, 2020

@francoisfreitag what is the status on this? I think this is a great feature!

Although I would change one part of the logic:

Currently in _after_postgeneration it will save() everytime results is not empty. You propose to skip it completely. This is what I propose:

results are filled in various conditions:

  • In the case of Transformer, it should return None
  • In the case of other post_generation calls, they should return something if they expect the instance to be saved again.

Then I would change the _after_postgeneration to:

        if create and any(results.values()):
            # Some post-generation hooks ran, and may have modified us.
            instance.save()

@francoisfreitag
Copy link
Member Author

The first step is to rewrite the example for PostGenerationMethodCall (#622). Once it no longer relies on a second save() being called, this PR can move forward.

In the case of Transformer, it should return None

Transformer is meant to replace some post_generation hooks. It is not a post_generation hook, but is called before the instance is generated.

change one part of the logic [and save when at least one post_generation hook returned a value]

I don’t think we should save() based on what the post_generation method returned. If users want the model to be saved, they can call save() on the object. What is the expected gain?

@MRigal
Copy link
Contributor

MRigal commented Nov 13, 2020

I don’t think we should save() based on what the post_generation method returned. If users want the model to be saved, they can call save() on the object. What is the expected gain?

Well for now, we always save the model when a post-generation hook was called, because it could have modified the object. I think we should change it so that every post-generation hook can determine if the _after_postgeneration should save the model again or not.

Removes the need to save objects with a post generation hook twice to the database.

I made a suggestion to make your first assumption in the description of the ticket more likely to happen.

Copy link
Member

@jdufresne jdufresne left a comment

Choose a reason for hiding this comment

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

Overall, I think this is a great idea. I find the save per postgeneration hook to frequently be unnecessary overhead. Putting that control in hands of users is welcome, I think.

I'm a bit concerned that without being careful, some users will end up with differences in memory and their persistent storage system. But I think the tradeoff is ultimately worth it. As resolving this concern takes some attention, I agree that we increase a full revision.

factory/declarations.py Outdated Show resolved Hide resolved
tests/test_transformer.py Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
@francoisfreitag
Copy link
Member Author

Changes:

  • rebase on master (mostly port to Python 3 and resolve conflicts)
  • add a “breaking changes” section to the release note
  • simplify tests a bit
  • add some comments to the Django Password example
  • a few wording updates

@francoisfreitag
Copy link
Member Author

Rebased on master with some tweaks to the changelog to make it clearer.

I suggest squashing the changes in new commit “Deprecation plan”. I’m sure the change is going to cause disruption for users. The deprecation plan allows warning users about the upcoming disruption, giving them time adapt. What do you think?

@francoisfreitag
Copy link
Member Author

Without further feedback, I’ll take a last look at this patch in January and merge it.

I’m happy to delay the merge if someone needs more time to review?

Transforms a value using provided `transform` function. Values coming
from the declaration and values overridden through keywords arguments
are transformed before the generated object attribute is set.

Removes the need to save objects with a post generation hook twice to
the database.

Facilitates overriding Django passwords when instantiating the factory.

Fixes FactoryBoy#316
Fixes FactoryBoy#366
@francoisfreitag francoisfreitag merged commit e083ff5 into FactoryBoy:master Feb 12, 2021
@francoisfreitag francoisfreitag deleted the 366_django_passwords branch February 12, 2021 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a simpler way to pre-compute passwords for Django save() method called twice when using post_generation
3 participants