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

Dynamically import modules of extensions #229

Merged
merged 1 commit into from
Feb 19, 2019
Merged

Conversation

ayan-b
Copy link
Member

@ayan-b ayan-b commented Feb 18, 2019

Related to #90

Before raising the PR, here is a check list:

  1. have you written unit tests for your code changes?
  2. have you run "make format"?
  3. are you requesting to "dev"?
  4. have you updated the change log?
  5. do you think that you can understand your changes after 6 month?
    5.1) can someone else understand your changes without your explanation?
  6. are you pround of your code changes?
    6.1) do you have the feeling of achievement?
  7. please add your name and github link to contributors.rst in alphabetical order.

@ayan-b ayan-b requested review from chfw and jayvdb February 18, 2019 13:46
@ayan-b
Copy link
Member Author

ayan-b commented Feb 18, 2019

Adding tests.

@codecov-io
Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #229 into dev will decrease coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
tests/test_docs.py 100% <100%> (ø) ⬆️
tests/test_engine.py 100% <100%> (ø) ⬆️
moban/jinja2/engine.py 100% <100%> (ø) ⬆️
tests/test_reporter.py 98.33% <0%> (-1.67%) ⬇️
moban/mobanfile/__init__.py 97.63% <0%> (-1.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26ac367...e44879d. Read the comment docs.

Copy link
Member

@chfw chfw left a 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?

@ayan-b
Copy link
Member Author

ayan-b commented Feb 18, 2019

Do we care about the return values of imported module?

No, I guess. If we only import, that'll do the job.

@chfw
Copy link
Member

chfw commented Feb 18, 2019

So we just remove globals thingy then we are good to go

Copy link
Member

@chfw chfw left a 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.

@ayan-b
Copy link
Member Author

ayan-b commented Feb 19, 2019

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/ .

@chfw
Copy link
Member

chfw commented Feb 19, 2019

OK. Will merge now. Please remember to update the docs :)

@chfw
Copy link
Member

chfw commented Feb 19, 2019

change log, change log update please.



def import_module_of_extension(extensions):
modules = set()
Copy link
Member

Choose a reason for hiding this comment

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

good use of set.

@ayan-b ayan-b self-assigned this Feb 19, 2019
@ayan-b ayan-b force-pushed the import-ext branch 2 times, most recently from 73a3ea6 to 0c1f489 Compare February 19, 2019 11:48
@ayan-b
Copy link
Member Author

ayan-b commented Feb 19, 2019

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/ .

Tried using jinja2-python-version, but it seems it does not support Python 2 and pypy.

Related build: https://travis-ci.org/ayan-b/moban/builds/495392863

Copy link
Member

@chfw chfw left a comment

Choose a reason for hiding this comment

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

Good to go!

@chfw chfw merged commit 4a69ad6 into moremoban:dev Feb 19, 2019
@ayan-b ayan-b deleted the import-ext branch February 19, 2019 18:34
ayan-b added a commit to ayan-b/moban that referenced this pull request Apr 21, 2019
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.

3 participants