-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Dynamically import modules of extensions #229
Conversation
Adding tests. |
Codecov Report
@@ Coverage Diff @@
## dev #229 +/- ##
==========================================
- Coverage 99.28% 99.16% -0.13%
==========================================
Files 57 57
Lines 2251 2271 +20
==========================================
+ Hits 2235 2252 +17
- Misses 16 19 +3
Continue to review full report at Codecov.
|
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.
Do we care about the return values of imported module?
No, I guess. If we only import, that'll do the job. |
So we just remove globals thingy then we are good to go |
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.
Can you update the docs where jinja2 extension used? Put an example to use jinja2 time module so that we are covered on integration and regression tests . Meanwhile, we got a bit more docs.
Last thing, please update changelog.
In order to use jinja2_time, we need to finish #202 (Ref). Instead, I am writing the docs using https://pypi.org/project/jinja2-python-version/ . |
OK. Will merge now. Please remember to update the docs :) |
change log, change log update please. |
|
||
|
||
def import_module_of_extension(extensions): | ||
modules = set() |
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.
good use of set.
73a3ea6
to
0c1f489
Compare
Tried using Related build: https://travis-ci.org/ayan-b/moban/builds/495392863 |
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.
Good to go!
Related to #90
Before raising the PR, here is a check list:
5.1) can someone else understand your changes without your explanation?
6.1) do you have the feeling of achievement?