-
Notifications
You must be signed in to change notification settings - Fork 23
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
Refactor to reduce duplication in post_gen hooks, plus other fixes #369
Conversation
ab160a6
to
3085544
Compare
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.
Looks pretty good! Some make upgrade calls we can get rid of though, I think. (Haven't tried running tests without them because yeah, it takes a lot longer now.)
...ter-django-ida/{{cookiecutter.repo_name}}/{{cookiecutter.project_name}}/settings/devstack.py
Outdated
Show resolved
Hide resolved
3085544
to
a70363c
Compare
@timmc-edx ready to go. |
tests/test_cookiecutter_xblock.py
Outdated
@@ -60,6 +60,8 @@ def fixture_options_baked(cookies_session, request, custom_template): | |||
baked result. | |||
""" | |||
with bake_in_temp_dir(cookies_session, extra_context=request.param, template=custom_template): | |||
sh.make('upgrade') | |||
sh.pip('install', '-r', 'requirements/base.txt') |
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.
Just noticing now that this was base.txt. Does it still work if we switch it to test.txt? 🤞
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.
Changed. It works. Now all four copies of this fixture use test.txt.
a70363c
to
9da60b3
Compare
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.
Awesome! Big improvement.
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.
errr... actually clicking "Approve" this time
We can split this into separate pull requests if needed. I got a bit aggressive with some changes....
Merge checklist:
Check off if complete or not applicable: