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

Add support for custom tags having access to extra data #50

Merged
merged 2 commits into from Nov 1, 2012

Conversation

Projects
None yet
2 participants
Contributor

saleyn commented Oct 26, 2012

RenderOptions passed to the template's render/2 function are propagated to a custom tag handling function, if the RenderOptions don't contain custom_tags_context optiotn. Otherwise the functionality is the same as before. This allows custom tags to have access to all options (e.g. locale) provided by the caller at render-time.

Contributor

saleyn commented Oct 26, 2012

Evan, we discussed this in the issue tracker. I implemented it so that custom_tags module can define a handling function having anywhere from 1 to 5 arguments depending on what render options the function needs to access. Added test cases for all custom tag options.

Contributor

evanmiller commented Oct 28, 2012

Hmm. I wonder if we should get rid of custom_tags_context and just pass in RenderOptions to custom tags instead. That way you'll get locale, translation_fun, and custom options without having to do anything special.

What do you think?

Contributor

saleyn commented Oct 29, 2012

That thought also crossed my mind. Though it would be a non-backward compatible change, so I didn't pursue it. However, it does make much more sense than parsing out a set of fixed arguments to be passed to a custom tags handling function. Here's what it would look like in a snippet of template's compiled code:

render(_Variables, Options) ->
    try render_internal(_Variables, Options)
    of
      Val -> {ok, Val}
    catch
      Err -> {error, Err}
    end.

render_internal(_Variables, _RenderOptions) ->
    TranslationFun = proplists:get_value(translation_fun, Options, none),
    CurrentLocale = proplists:get_value(locale, Options, none),
    ...
    some_custom_tag_fun(_Variables, _RenderOptions),
    ...
Contributor

evanmiller commented Oct 29, 2012

What we could do is see if "custom_tags_context" is present. If yes, pass that in as arg #2. If no, pass all RenderOptions as arg #2. That way we are backward-compatible but can transition to a more sensible API.

Contributor

saleyn commented Oct 29, 2012

Though the problem with that is that from the point of the custom tag handling function it would have no way of verifying if what's passed as its 2nd argument is the CustomTagContext or RenderOptions. It doesn't seem elegant to change the meaning/content of the argument based on the value contained within. Maybe a better approach would be to have a configuration option has_custom_tag_context, which if true (default) would pass CustomTagContext, and if false, pass RenderOptions.
Also it's worth mentioning that the custom tags handling logic unlikely worked correctly in the legacy code - there was a bug (which I fixed in the second commit in this pull request), which resulted in the "FIle not found" error when passing a custom_tags_modules option - erlydtl would try to search a custom_tags_dir for a file implementing a custom tag and would fail with "File not found" error before getting a chance to process custom_tags_modules. So it looks to me that "breaking" backwards compatibility (by choosing a default for has_custom_tag_context to be false in the suggestion above) wouldn't be an issue.

Contributor

evanmiller commented Oct 30, 2012

"has_custom_tag_context" is needlessly complex in my opinion. Why does the custom tag handler need to "know" what it's being passed? I think we should make it clear that custom_tags_context is (will be) deprecated and only provided for backwards compatibility.

Contributor

saleyn commented Oct 30, 2012

ok, I'll update the patch.

saleyn added some commits Oct 23, 2012

Add support for custom tags having access to extra data
The following arguments are conditionally exposed to custom tags:
* RenderVariables
* Locale
* TranslationFun
Fix "File not found" error
When a custom tags implementing module is defined in `custom_tags_modules`,
don't throw an exception if the `custom_tags_dir` doesn't contain the file
implementing the custom tag.
Owner

saleyn commented on 6e7bb36 Oct 30, 2012

Patch updated as discussed. All tests pass, and decompiled template code looks kosher.

Contributor

evanmiller commented Oct 31, 2012

Great. My only quibble is with "atom_to_binary" -- do you know when this was introduced? I think this is a new-ish BIF and I want to make sure ErlyDTL continues to run on older installations. The README doesn't advertise a particular version but the last time I checked (a long time ago) we worked on R13 and maybe even R12.

Contributor

saleyn commented Nov 1, 2012

According to the otp github archive, the BIF was present in the R13B03:
https://github.com/saleyn/otp/blob/84adefa331c4159d432d22840663c38f155cd4c1/lib/compiler/src/erl_bifs.erl#L66

Unfortunately they don't have history past that and I upgraded all my installs to R15B02, so I am unable to check if it existed prior to that release. So, I'd say that it's safe to assume that the majority of ErlyDTL users should have no problems with that BIF, or otherwise should upgrade their Erlang installation.

Contributor

evanmiller commented Nov 1, 2012

Ok, good enough for me then.

Thanks for all the hard work on this!!

evanmiller added a commit that referenced this pull request Nov 1, 2012

Merge pull request #50 from saleyn/locale
Add support for custom tags having access to extra data

@evanmiller evanmiller merged commit 28c946e into erlydtl:master Nov 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment