-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
berinhard
merged 6 commits into
model-bakers:main
from
timjklein36:tk/52-foreign-key-string-recipe
Nov 8, 2020
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
3960266
[52] Add ability to load foreign_key recipe argument from another module
timjk-gp 7026006
[52] Add unit tests for get_calling_module
timjk-gp fe05401
[52] Add unit tests for loading recipe from other module
timjk-gp 6dff184
[52] Update CHANGELOG.md
timjk-gp c048cad
[52] Add some type `cast` calls to appease mypy
timjk-gp 175e3ea
[52] Change wording: `stacks` -> `frames`
timjk-gp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
There's a function in
utils.py
namedimport_from_str
. It's used bybaker.make
orbaker.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.
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.
So, this functionality was already present in the import logic of
RecipeForeignKey
and ofrelated
. 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!
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.
For context, I did re-use the
baker._recipe
import function (which callsimport_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 theforeign_key
function).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.
@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.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.
@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.