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

Support for equality operators #73

Closed
wants to merge 2 commits into from

Conversation

moritzgloeckl
Copy link

@moritzgloeckl moritzgloeckl commented Nov 4, 2017

I needed a simple way to add text depending on the value of the conditional, so I added this functionality.

It works like this:

«technology:if(=='Ruby')»
    ... arbitrary document markup ...
«technologies:endIf»

«age:if(!=45)»
    ... arbitrary document markup ...
«age:endIf»

I modified the README as well and also added a test case. Feedback? Thanks

@senny
Copy link
Owner

senny commented Nov 4, 2017

Hi @moritzgloeckl

I am not sure that this is something that should be merged into sablon. Let me give you a bit of background to explain my reasoning.

Sablon was designed with the idea that templates are cumbersome to work with. It's awkward to edit them and you can't see diffs in source control. For that reason, the expression language in sablon is very minimal. The way you work around this limitation is by defining methods on your context object. To tie this back to your example, you would have a method is_ruby? or is_not_forty_five?. Not only does this deliver the same functionality for your templates, it puts more knowledge into your code. Code is much easier to work with than the templates.

tl;dr while there are definitely cases where more complex conditionals are required. The idea behind sablon is to wrap that logic in methods on the context object to push complexity towards your codebase and not the templates.

Thank you for your time. I'm interested to hear about your use-case and if defining methods is not a feasible approach.

@stadelmanma
Copy link
Collaborator

I definitely agree with @senny on that we want templates to be simple. In my project I pushed everything to HTML using ERB templates and then do simple content substitutions into my actual word docs, so that may be a route worth exploring depending on the constraints of your project if you need more complex logic.

However, I'm still 50/50 on the idea of supporting operators since on a level it does seem silly to need methods to check for simple logical conditions (==, >, !=, etc.). Although, if we would move forward with this I think the current implementation would need tweaked some.

@moritzgloeckl
Copy link
Author

Hi @senny,

thanks for your answer! Your argument sounds reasonable, and you are very correct about the debugging. Let me tell you about my use-case and why I added this functionality.

In my case my frontend sends a JSON to my rails backend. There's no user authentication involved, so if you're sending the correct JSON you get a correct document back. My controller just gives the JSON to Sablon and let it figure out which text to display depending on the conditionals and the values. This way I keep my backend small and have a single point where I can edit the template if needed.

I think you could do it either way, I just figured maybe someone else wanted a feature like this, but it's totally fine not to merge it.

Thank you very much for your answer!

@stadelmanma
Copy link
Collaborator

stadelmanma commented Mar 22, 2018

Hi @moritzgloeckl, I assume you kept using your fork of the gem to support this feature but the recently merged PR will allow you to accomplish this without forking the gem. I used part of the code you submitted in this PR in the test case since I thought it would serve as a very good example. I'm not going to cut a release yet but you can pull the gem from the master branch to try things out.

See the custom_field_handlers_test.rb file for an example on how to add your own merge fields to Sablon.

Thanks for your interest in the gem!

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