Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Jinja autoload filters #30

Merged
merged 3 commits into from Nov 8, 2012

Conversation

Projects
None yet
3 participants
Contributor

clsdaniel commented Oct 3, 2012

Added code to prevent jinja for loading all the functions inside the jinja_filters.py file and thus polluting the jinja filter namespace.

This patch corresponds to a discussion thread on the list:

https://groups.google.com/forum/?fromgroups=#!topic/turbogears/Auk-JL0nrc8

I did this on September and tried to rebase my branch I don't know why the pull requests adds another commit which is not mine (from Alessandro) which is already in the tg2 devel branch.

clsdaniel and others added some commits Sep 29, 2012

When autoloading jinja filters use the special __all__ module variabl…
…e to import the filters, if it is not defined then just import any defined callables.
Owner

pedersen commented Oct 3, 2012

@amol- Could you check through this? r:51e3826 looks like it could be adding Pylons dependencies in, and I'm pretty sure you were trying to remove those dependencies.

Owner

amol- commented Oct 3, 2012

Are your referring to the beaker_cache part?
As it is one of the core functions in TurboGears2 caching on 2.3 branch @beaker_cache is part of TurboGears2 itself and is available inside the tg.decorators module.

To make life easier for people when moving forward from 2.2 to 2.3 I added the @beaker_cache inside tg.decorators in 2.2 too, this way people won't need to import it inside their application from the pylons namespace and when moving forward won't have issues related to the pylons namespace not being available anymore.

I hope I have been clear :D
Summarizing I added a dependency to pylons inside TurboGears2 so that people won't have it inside their own app.

Owner

pedersen commented Oct 3, 2012

Yep, that is what I was referring to. Now that i know that, I can review the rest of this tonight (or you can) for merge. Thank you!

EDIT: @clsdaniel, I hate to be annoying, but I don't see any test cases for this. Can you add at least one in?

Owner

pedersen commented Oct 15, 2012

@clsdaniel I think my last edit failed to notify you. I want to do this merge, but don't see any test cases in the pull request you submitted. Can you add some, so we can merge this in?

Thanks!

Contributor

clsdaniel commented Oct 15, 2012

@pedersen My bad, added appropriate tests cases, which was a bit trickier than expected; if the new autoloading code works fine functions not defined in the special variable "_ all _" will throw an exception when used in the template (expected behaviour), which is what the test case looks for.

Contributor

clsdaniel commented Oct 16, 2012

@pedersen Seems like I had the same problem with notifications, just replying again, the tests are now included in the pull requests and travis has already run the build ok.

amol- added a commit that referenced this pull request Nov 8, 2012

@amol- amol- merged commit 8af6bf8 into TurboGears:development Nov 8, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment