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

[52] Allow string values for foriegn_key from other modules #120

Merged

Conversation

timjklein36
Copy link
Collaborator

@timjklein36 timjklein36 commented Oct 14, 2020

What

This change set allows a caller of foreign_key to supply a recipe via a str value that refers to a recipe from another module. Up to now, if a str value was passed in to foreign_key it would attempt to load that recipe in the module from which foreign_key was called. That functionality is still present. It is used as a fallback if the str value provided does not refer to a recipe from another module.

Fixes #52.

Why

This offers more flexibility to the use of foreign_key and does not require the caller to import the recipe that they want to use.

How

The code for foreign_key, RecipeForeignKey, and related was refactored slightly to reduce code duplication for the logic of loading a recipe from the "calling module". In addition to that, since it is assumed that most users of model_bakery call foreign_key and do not instantiate RecipeForeignKey directly, the logic for loading a recipe from a given module path string and the fallback logic for loading from the "calling module" string were placed inside foreign_key and removed from RecipeForeignKey's __init__ method to make it cleaner. The __init__ method now only accepts Recipe objects.

Docstrings were also added to some existing functions/classes and all new functions.

Example Usage

people.baker_recipes.py

person = Recipe(
    Person,
    name="John Doe"
)

dogs.baker_recipes.py

dog = Recipe(Dog, breed="Pug", owner=foreign_key("people.person"))

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.

The PR is great @timjklein36! Thanks again for contributing to the project.

I left a comment about an existing helper function to handle string imports. I'd like to know if you have time to take a lookt at it and to check if it's possible to refactor your PR to use it instead of adding new import strategies.

Comment on lines +26 to +39
def get_calling_module(levels_back: int) -> Optional[ModuleType]:
"""Get the module some number of stack frames back from the current one.

Make sure to account for the number of frames between the "calling" code
and the one that calls this function.

Args:
levels_back (int): Number of stack frames back from the current

Returns:
(ModuleType): the module from which the code was called
"""
frame = inspect.stack()[levels_back + 1][0]
return inspect.getmodule(frame)
Copy link
Member

Choose a reason for hiding this comment

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

There's a function in utils.py named import_from_str. It's used by baker.make or baker.prepare to work with string paths instead of model classes.

This PR looks great and it definitely makes bakery more stable in terms of its external API. But I wonder if we could re-use the previous import function instead of defining new strategies to import models based on strings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, this functionality was already present in the import logic of RecipeForeignKey and of related. I simply extracted it out into a re-usable function.

I personally think the logic here (using stack frames to get the module from which the code was called) is a bit fragile, but it was already the way it was implemented so I left it alone (aside from pulling it into its own function to avoid duplication).

I am definitely open to alternatives, if you think there is a better way to access the "calling module" and its attributes. Just let me know what you think, and I will update the code. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For context, I did re-use the baker._recipe import function (which calls import_from_str) for the new logic that I added, however, I left the "import from calling module" logic as a fallback (to maintain existing use of the foreign_key function).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@berinhard Please let me know if there is a way to facilitate loading a recipe from the module that called foreign_key that can re-use any existing function.

Copy link
Member

Choose a reason for hiding this comment

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

@timjklein36 sorry I didn't have the time to properly review this last week. But I'll do so today and will update the PR with comments.

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.

@timjklein36 thanks for your PR! I reviewed it with more time and calm now and it looks great!

I'd also like to apologize myself for the loose review I gave to this PR in the past and my comments because they were not valid and were created due my lack of concentration when reviewing this. I honestly hope to keep reviewing new PRs from you =)

@berinhard berinhard merged commit c52d95a into model-bakers:main Nov 8, 2020
@timjklein36
Copy link
Collaborator Author

@berinhard No problem! I enjoy contributing. Thanks for the review!

@timjklein36 timjklein36 deleted the tk/52-foreign-key-string-recipe branch November 8, 2020 16:15
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.

Allow string values for foriegn_key from other modules
3 participants