Issue 2180 - [cms] Add support for custom Jinja2 functions

Issue 2180 - [cms] Add support for custom Jinja2 functions

[Sebastian Noack, 2015-03-20 09:49:43]

http://codereview.adblockplus.org/5521995252891648/diff/5741031244955648/cms/...
File cms/converters.py (right):

http://codereview.adblockplus.org/5521995252891648/diff/5741031244955648/cms/...
cms/converters.py:271: for dirname, dictionary in [("filters", filters),
("functions", globals)]:
How about calling it "globals" as well to match the jinja2 API, and strictly
those doesn't need to be limitted to functions anyway.

Also this patch should update the README as well.

[Wladimir Palant, 2015-03-20 16:02:24]

http://codereview.adblockplus.org/5521995252891648/diff/5741031244955648/cms/...
File cms/converters.py (right):

http://codereview.adblockplus.org/5521995252891648/diff/5741031244955648/cms/...
cms/converters.py:271: for dirname, dictionary in [("filters", filters),
("functions", globals)]:
On 2015/03/20 09:49:43, Sebastian Noack wrote:
> How about calling it "globals" as well to match the jinja2 API, and strictly
> those doesn't need to be limitted to functions anyway.

Strictly speaking - yes, these don't need to be functions. It is hard to imagine
what else they could be however. Either way, fine with me.

> Also this patch should update the README as well.

Sure.

[Sebastian Noack, 2015-03-20 16:10:54]

http://codereview.adblockplus.org/5521995252891648/diff/5741031244955648/cms/...
File cms/converters.py (right):

http://codereview.adblockplus.org/5521995252891648/diff/5741031244955648/cms/...
cms/converters.py:271: for dirname, dictionary in [("filters", filters),
("functions", globals)]:
On 2015/03/20 16:02:24, Wladimir Palant wrote:
> On 2015/03/20 09:49:43, Sebastian Noack wrote:
> > How about calling it "globals" as well to match the jinja2 API, and strictly
> > those doesn't need to be limitted to functions anyway.
> 
> Strictly speaking - yes, these don't need to be functions. It is hard to
imagine
> what else they could be however. Either way, fine with me.

Instances (with methods) would come to my mind.

> > Also this patch should update the README as well.
> 
> Sure.

Thanks.

http://codereview.adblockplus.org/5521995252891648/diff/5757334940811264/cms/...
File cms/converters.py (right):

http://codereview.adblockplus.org/5521995252891648/diff/5757334940811264/cms/...
cms/converters.py:286: if hasattr(dictionary[name], "__call__"):
You might want to use the callable() built-in function instead. But how does it
come that this hack is necessary?

[Wladimir Palant, 2015-03-20 16:21:12]

http://codereview.adblockplus.org/5521995252891648/diff/5757334940811264/cms/...
File cms/converters.py (right):

http://codereview.adblockplus.org/5521995252891648/diff/5757334940811264/cms/...
cms/converters.py:286: if hasattr(dictionary[name], "__call__"):
On 2015/03/20 16:10:54, Sebastian Noack wrote:
> You might want to use the callable() built-in function instead. But how does
it
> come that this hack is necessary? 

A function doesn't usually have a reference to its module, and the modules we
load here aren't being added to any global table. So while the functions
themselves aren't garbage collected, the module might - and then the function
will suddenly be unable to access variables or imports from its module.

[Sebastian Noack, 2015-03-20 16:26:53]

http://codereview.adblockplus.org/5521995252891648/diff/5757334940811264/cms/...
File cms/converters.py (right):

http://codereview.adblockplus.org/5521995252891648/diff/5757334940811264/cms/...
cms/converters.py:286: if hasattr(dictionary[name], "__call__"):
On 2015/03/20 16:21:12, Wladimir Palant wrote:
> On 2015/03/20 16:10:54, Sebastian Noack wrote:
> > You might want to use the callable() built-in function instead. But how does
> it
> > come that this hack is necessary? 
> 
> A function doesn't usually have a reference to its module, and the modules we
> load here aren't being added to any global table. So while the functions
> themselves aren't garbage collected, the module might - and then the function
> will suddenly be unable to access variables or imports from its module.

Yeah, this actually make sense, since we create the module, as opposed to
importing it where a reference ends up in sys.modules. And since global
variables aren't bound to the closure of the function they get lost if there
isn't a reference to the module anymore. But that is certainly worth a more
verbose comment.

[Wladimir Palant, 2015-03-20 16:42:50]

http://codereview.adblockplus.org/5521995252891648/diff/5757334940811264/cms/...
File cms/converters.py (right):

http://codereview.adblockplus.org/5521995252891648/diff/5757334940811264/cms/...
cms/converters.py:286: if hasattr(dictionary[name], "__call__"):
On 2015/03/20 16:26:54, Sebastian Noack wrote:
> On 2015/03/20 16:21:12, Wladimir Palant wrote:
> > On 2015/03/20 16:10:54, Sebastian Noack wrote:
> > > You might want to use the callable() built-in function instead. But how
does
> > it
> > > come that this hack is necessary? 
> > 
> > A function doesn't usually have a reference to its module, and the modules
we
> > load here aren't being added to any global table. So while the functions
> > themselves aren't garbage collected, the module might - and then the
function
> > will suddenly be unable to access variables or imports from its module.
> 
> Yeah, this actually make sense, since we create the module, as opposed to
> importing it where a reference ends up in sys.modules. And since global
> variables aren't bound to the closure of the function they get lost if there
> isn't a reference to the module anymore. But that is certainly worth a more
> verbose comment.

Done.

[Sebastian Noack, 2015-03-20 16:44:44]

LGTM