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

Templating doesn't work with files that include gstring substitutions #40

Closed
kyleboon opened this issue Jun 29, 2013 · 15 comments
Closed

Comments

@kyleboon
Copy link
Contributor

If you apply the lazybones script to a file that uses a gstring in it, the lazybones script will always attempt to iterpolate that string, even if it is part of the normal source of the file.

For example, I have a build.gradle file with the following:

task migrate(dependsOn: 'shadow', group: 'dropwizard', description: "Run migrations using the dev configuration") << {
    javaexec {
        main = '-jar'
        args = ["${shadow.shadowJar.getPath()}", 'db', 'migrate', 'dev_config.yml']
    }
}

If the lazybones.groovy script includes filterFiles("build.gradle", filterProperties) then the SimpleTemplateEngine will try to interpolate "${shadow.shadowJar.getPath()}" resulting in an invalid build.gradle.

I have been working around this so far, but given that we're going to be templating groovy and gradle files a lot, I think SimpleTemplateEngine isn't going to work for us long term. What about using something like handlebars? There is a simple java implementation that I have used for server side templating.

@tbarker9
Copy link
Contributor

It works provided you escape everything properly. For example:

import groovy.text.*

def text = """
task migrate(dependsOn: 'shadow', group: 'dropwizard', description: "Run migrations using the dev configuration") << {
    javaexec {
        main = '-jar'
        args = ["\\\${shadow.shadowJar.getPath()}", 'db', 'migrate', 'dev_config.yml']
    }
}
"""


def template = new SimpleTemplateEngine().createTemplate(text).make()

assert template.toString().contains('["${shadow')

Given that I am dealing with text I need two more \ than I would need in a file. So the question is, is this too annoying to expect from template authors? Should we use something that is not groovy to encourage people outside of the jvm community to use this?

@tbarker9
Copy link
Contributor

I personally don't care about the extra escaping (@kyleboon please tell me if I am missing something here).... I do care about the second point though.

@pledbrook
Copy link
Owner

I'm tempted to offer multiple template options here. I can imagine Handlebars templates being equally annoying for Handlebars template files. SimpleTemplateEngine is also more familiar to people in the Groovy community than Handlebars.

There is a cost associated with increased flexibility, but I don't see the codebase becoming overly complicated by supporting multiple template engines. That said, I don't think Lazybones should have too many dependencies. The core should only include a maximum of 3 engines, preferably only 2. Thoughts?

@tbarker9
Copy link
Contributor

tbarker9 commented Jul 1, 2013

I haven't created enough templates to say this is worth it. That said I will likely use 100% groovy for all templates despite any shortcomings.... going to stick with what I know. Some solid documentation regarding escaped characters / gotchas might be worth some time though. Personally I think we should pursue a 100% groovy solution until we get enough complaints to do otherwise.

If we want this to be used outside of java / groovy land, supporting another template engine might be worthwhile. I suspect that might be a pipe dream though 😄

@tbarker9
Copy link
Contributor

tbarker9 commented Jul 1, 2013

One compromise I just thought of... make the root script and template engines extensible. Although the lazybones product will only support groovy out of the box, a template could contain jars / groovy files which are added to the classpath before the root script is fired off. After the template is successfully installed, these plugin files are deleted. Can't tell if this reeks of over engineering, but maybe something to keep in mind.

@tbarker9
Copy link
Contributor

tbarker9 commented Jul 5, 2013

I mentioned adding ivy in #36 to enable Grape. Maybe this could be the first step to support lazybones plugins / different template engines.

@kyleboon
Copy link
Contributor Author

kyleboon commented Jul 5, 2013

I like the idea of multiple template engines

@tbarker9
Copy link
Contributor

tbarker9 commented Jul 5, 2013

I am still unsure this is worth the effort, but if it did happen handlebars seems reasonable to me.

@pledbrook
Copy link
Owner

It looks like the Java Handlerbars implementation will add around 1.5MB to the distribution (mostly because of the dependency on Rhino). That makes it less than appealing as a core feature for me unless there is a great clamour for adding it.

That said, we could provide an SPI for template engines and let template authors include custom ones via Groovy's Grapes mechanism as @tbarker9 points out. The only difficulty is knowing what the SPI should look like with only one current implementation 😄

Is anyone interested in looking at an alternative template engine? That would help in determining what the SPI should look like.

@tbarker9
Copy link
Contributor

tbarker9 commented Jul 6, 2013

freemarker is a little less than a meg. No dependencies. Velocity is smaller but has a lot of dependencies. We could also make one of our own for demonstration purposes I suppose. For instance, I think some grails projects will replace anything between @, while not allowing programmatic configurations.... just find and replace.

@tbarker9
Copy link
Contributor

tbarker9 commented Jul 6, 2013

StringTemplate might also be an option, about a third of a meg with dependencies. It does have a dependency on antlr-runtime though. Not sure if that will cause some kind of conflict with the antlr libraries that groovy depends on.

@kyleboon
Copy link
Contributor Author

kyleboon commented Jul 6, 2013

Groovy has a Template Interface and an Abstract TemplateEngine class. Would we need anything else?

http://groovy.codehaus.org/api/groovy/text/Template.html

We could just load the template engine class based on a registration of file extensions to concrete TemplateEngine.

SimpleTemplateEngine, GStringTemplateEngine and XmlTemplateEngine are in the core groovy distribution so we wouldn't need to have any more dependencies. Then we could add a registerTemplateEngine(String extension, Class templateEngine) method in the Script base to register additional template engines.

@tbarker9
Copy link
Contributor

tbarker9 commented Jul 6, 2013

Oh nice.... I didn't even think of that.

So how exactly would this work? By default the SimpleTemplateEngine would handle all extensions and anything registered can override that. Or no matter what someone has to register a template engine? Do you think we should do something to auto discover the templates instead of explicitly adding it?

Right now we are filtering files in place, will that still continue? Or would we use file extensions specific to the template engine? So a file that is processed by the SimpleTemplateEngine might have the extension gtmpl. So assuming we have the build file build.gradle.gtmpl, it will be processed and deleted, with the final processed file being build.gradle.

@pledbrook
Copy link
Owner

I would stick to passing an engine instance into the filterFiles() method. We can leave file extension registration to later if it appears to be a valuable feature. Same goes for auto-discovery. I think it's better for template authors to explicitly declare what files they want filtering.

I think it's nice to support dedicated file extensions for template files. They make it clear what's a template and what's not in the source. The only question I have in my mind is whether it makes sense to support both that approach and in-place filtering. I'd prefer to only support one of them to ensure consistency, but I don't have a strong opinion either way. Thoughts?

@pledbrook
Copy link
Owner

Moving the discussion to the more focused issue #52.

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

No branches or pull requests

3 participants