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

Enable seq to be imported from baker #76

Merged
merged 1 commit into from
Jun 14, 2020

Conversation

radwon
Copy link
Contributor

@radwon radwon commented Jun 4, 2020

Enable the seq() function to be imported from the baker module like
it is in the recipe module so that it can be used as baker.seq() if
preferred or to avoid namespace issues.

The current import can be left in place for backwards compatibility
or to continue importing seq as before.

Closes #75

Copy link
Collaborator

@amureki amureki left a comment

Choose a reason for hiding this comment

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

Thanks for a good proposal @radwon !

I am wondering if we should consider following the same pattern for other things that we often import, for example stuff from recipe module - recipe.Recipe, recipe.foreign_key etc.

Then maybe we can think about better unifying the whole thing maybe, otherwise we'd have too many possibilities (which is not always a good practice):

from model_bakery import baker, recipe

baker.seq("test")
recipe.seq("test")
baker.Recipe("mymodel")
recipe.Recipe("mymodel")

Anyways, I'd love to hear other opinions here before going forward.
@radwon @anapaulagomes @berinhard what do you think?

@anapaulagomes
Copy link
Contributor

Sounds good to me, @amureki. @radwon, thank you for your contribution! 🎉

@berinhard
Copy link
Member

I agree with your suggestion @amureki for the Recipe as well! Can you document it on a new issue?

Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

@radwon this PR looks great! Can I ask you just for 2 minor things?

  1. Add a simple unit test to ensure the baker.seq is working as expected. Maybe a test very similar to this one;
  2. Update the CHANGELOG.md file with this addition;

And thanks for being careful on updating the docs as well =)

Enable the seq() function to be imported from the baker module like
it is in the recipe module so that it can be used as baker.seq() if
preferred or to avoid namespace issues.

The current import can be left in place for backwards compatibility
or to continue importing seq as before.

Closes model-bakers#75
@radwon
Copy link
Contributor Author

radwon commented Jun 14, 2020

@berinhard I've made the requested changes, and also made some changes to test_recipes.py renaming self.fail to pytest.fail as the TestDefiningRecipes class doesn't actually have a fail method.

Hope that's OK. Let me know if there's anything else.

Copy link
Member

@berinhard berinhard left a comment

Choose a reason for hiding this comment

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

Thanks @radwon! It looks great =)

@berinhard
Copy link
Member

I'm merging since @anapaulagomes had already approved this previously and @radwon only added tests to make sure the changes he'd introduced are working.

@berinhard berinhard merged commit 14630fb into model-bakers:master Jun 14, 2020
amureki pushed a commit that referenced this pull request Jun 14, 2020
* origin/master:
  Enable seq to be imported from baker (#76)
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.

Enable seq to be imported from baker
4 participants