new arg --templates for parsing additional templates

[laher]

Does this seem like a good approach for loading additional templates like {{ template "otherfile.tpl" }}?

I think couldn't think what to call the CLI arg - maybe --additional-templates is better, but here's a straw man.

Cheers, great project, just this one thing missing IMO 👍 💯 ❤️

[hairyhenderson]

Hi @laher, thanks for the PR!

I definitely want to implement support for nested templates, and there's already some discussion in #210 about the right way to implement that support.

So far the approach I'm leaning towards is this one: #210 (comment)

Would you mind commenting on that issue, and letting me know whether that approach would work for you, or if you have reasons that it wouldn't?

[laher]

Interesting, thanks. I don't get why you'd treat templates as data, they're exactly not the data. I'll reply later today in that thread with a more thoughtful response :)

…

On Wed, Sep 19, 2018, 9:07 AM Dave Henderson ***@***.***> wrote: Hi @laher <https://github.com/laher>, thanks for the PR! I definitely want to implement support for nested templates, and there's already some discussion in #210 <#210> about the right way to implement that support. So far the approach I'm leaning towards is this one: #210 (comment) <#210 (comment)> Would you mind commenting on that issue, and letting me know whether that approach would work for you, or if you have reasons that it wouldn't? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#403 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHrlcXxcthIwebthA2G92tsCbIOXm3yks5ucWCjgaJpZM4Wul4l> .

[laher]

@hairyhenderson as discussed, I have implemented those local-filesystem use cases 1-4.

I did do something differently, but I can change it if it doesn't work for you: I found that the standard library template.ParseFiles(...) uses the basename of the file as the template name, so it felt more natural to follow suit with that.

i.e. instead of this: gomplate --template path/to/foo.t -i '{{ template "path/to/foo.t" }}'
it does this: gomplate --template path/to/foo.t -i '{{ template "foo.t" }}'

and instead of: gomplate --template path/to/ -i '{{template "path/to/foo.t" }}'
it does: gomplate --template path/to/ -i '{{template "foo.t" }}'

I did do the directory-aliasing prefix though: gomplate --template bar=path/to/ -i '{{template "bar/foo.t" }}', and I took out the globbing.

What do you think?

[hairyhenderson]

I found that the standard library template.ParseFiles(...) uses the basename of the file as the template name, so it felt more natural to follow suit with that.

Yeah - I was a bit conflicted about that. The advantage is, of course, a shorter template name, but the disadvantage is that templates with the same filename in different directories will conflict. It's a relatively minority edge-case, but I'd rather be more explicit about the name, especially when you consider that you can always use an alias like --template foo.t=path/to/foo.t to achieve the same effect.

Keeping the path as-is is also more in-line with what you get when you reference a directory, so I think I'd rather the internal consistency over the template.ParseFiles behaviour.

[hairyhenderson]

you'll need to add // nolint: gosec above this line to get the linter to shut up:

template.go:43::warning: Potential file inclusion via variable,MEDIUM,HIGH (gosec)

It's more than a potential file inclusion, it's a deliberate file inclusion, but that's the whole point of this feature 😂

[hairyhenderson]

@laher Further to the comment above, this looks good so far, thank you for working on this!

And when you get the CI build to pass (should just be a case of appeasing the linter - see inline comments about that), I'll get this merged.

I'll follow up later with unit & integration tests, unless you want to add some yourself.

This'll also need some changes to documentation - but again, I can take care of that unless you feel like it 😉. It can certainly happen in another PR.

[hairyhenderson]

Just a few comments - see inline. Thanks for doing this work!

[laher]

I got the CI passing, simplified parseTemplateArgs, and fixed some bugs in the process (mainly regarding slashes, paths & aliases)

Re testing: I've been testing manually on the CLI, it seems to work OK, but I think I'd need to rework the code in order to support the afero fs as in TestGatherTemplates. Not sure the best way to approach it at this point, and I'm not gonna get back to this for a few days now... Cheers

[hairyhenderson]

Thanks @laher - this is great, and I'll merge it at this point. I totally understand needing to switch contexts for a few days, you've already been very generous with your time.

I'll merge this now, and will add some tests and docs for this.