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 'include' and 'inheritance' #7

Closed
wants to merge 8 commits into from

Conversation

tandiljuan
Copy link
Contributor

These changes add support for:

@chevdor
Copy link
Owner

chevdor commented Aug 23, 2021

Very nice, thanks @tandiljuan !
I will test is shortly but here is already a "thank you!" and a few comments.

Would you mind adding a short word/example about the new feature in the readme ?

This is done in the adoc file. The readme.md is generated with just md.

I find --patha little vague.
What do you think about renaming --path to --include or --include-path ?

@chevdor
Copy link
Owner

chevdor commented Aug 23, 2021

It will be time I add benchmarks :)

I tested the following:

time ./target/release/tera --template data/polkadot-metadata/metdata.tera data/polkadot-metadata/v13.json

and (where tera is the latest release:

time tera --template data/polkadot-metadata/metdata.tera data/polkadot-metadata/v13.json

With the latest release, on my machine, the run takes around 9ms.
With your PR, I get to 70~75ms which is significantly more which makes me think that it may not be optimum for big templates/data to enable your include/path feature by default.

I think it might be better to keep this new --path or --include flag fully opt-in and not have anything more done if --path or --include is not passed by the user.

@tandiljuan
Copy link
Contributor Author

Hi @chevdor !

All the suggestions seems good to me.

I'll work on them ASAP (probably in the weekend) and I'll let you know once I have the update ready.

Kind regards.

@chevdor
Copy link
Owner

chevdor commented Aug 23, 2021

Sure, no stress but I am looking forward to have this merged in. It was also on my list so very nice that you did it.
I also welcome the (althought not related :)) changes to the release profile.

@tandiljuan
Copy link
Contributor Author

tandiljuan commented Sep 3, 2021

Hi @chevdor ! I did the requested updates. Could you please check if everything is okay? Thanks!

@chevdor
Copy link
Owner

chevdor commented Sep 3, 2021

Not sure I will manage today but I'll see if I can squeeze it in. Thanks a lot for your time on this!

@chevdor
Copy link
Owner

chevdor commented Sep 3, 2021

Short comment, I will you squeezed more features in. In general, it would be best to split them in different PRs so interesting features are not blocked by say.. some docker grumbles. Leave it as it is for now and I will check it as it is. Just keep in mind that I may ask you to split :)

@@ -0,0 +1,46 @@
FROM rust:1.54.0-slim-buster as tera
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally a non issue but the builder image stage in multistage images is usually called builder.

Copy link
Owner

@chevdor chevdor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work !

Considering the number of comments I made regarding Docker, I think it may be better to split this PR and move the Docker part to another PR so we can already merge the first one.

So regarding the --include part of the PR, I think it would be great adding a simple example under data/include for instance. That makes manual testing easier, helps to add cli tests later, and I can also pull the example into the doc.

Feel free to include the example to the .adoc yourself if you fancy playing with asciidoc includes but I would ask you to do so only if you can run just md yourself to update the markdown. Otherwise, leave it and I will take care of it later.

As far as the example is concerned, we can keep it dead simple with:

./data/include/hello.txt:

Hello {{ USER }}, this is the Hello template.
{% include "world.txt" %}

./data/include/world.txt:

This is the world template.

and documenting the call as:

tera -t ./data/include/hello.txt --include --env-only


=== Build container image

docker build --rm --tag 'tera/cli' .
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does --rm do in that context ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove intermediate containers after a successful build. The default is true.
But, taking in mind the default is true, we can remove it.

Parse a template

docker run --interactive --tty --rm \
--volume="$(pwd):/opt" \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why mounting this volume ?
It does not seem to be used after.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed to mounts the templates into the container and be able to parse them.

@@ -56,6 +56,25 @@ The **tera** engine allows way more than the simple replacements shown above. Yo

cargo install --git https://github.com/chevdor/tera-cli

== Execute from container
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
== Execute from container
== Execute as Docker container

# Install dependencies
# ******************************************************************************
RUN set -eux \
&& DEBIAN_FRONTEND=noninteractive apt-get update \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather suggest we add:

ARG DEBIAN_FRONTEND=noninteractive

before this RUN and remove all the DEBIAN_FRONTEND=noninteractive from the RUN. That will avoid a lot of repetition.


=== Build container image

docker build --rm --tag 'tera/cli' .
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
docker build --rm --tag 'tera/cli' .
docker build --tag tera-cli .

We do not own the tera docker user so let's avoid it. People can then run:

docker tag tera-cli tandiljuan/tera-cli

and push it if they want.


docker build --rm --tag 'tera/cli' .

=== Execute `tera` from container
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
=== Execute `tera` from container
=== Execute `tera` from the Docker container


Check tera help

docker run --interactive --tty --rm tera/cli --help
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
docker run --interactive --tty --rm tera/cli --help
docker run -it --rm tera-cli --help

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like to use long options in documentation to make them more descriptive. But If you prefer the short version, there is no problem.

docker run --interactive --tty --rm \
--volume="$(pwd):/opt" \
--env=FOO=BAR \
tera/cli --template templates/env-debug.txt --env-only --env-key env
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tera/cli --template templates/env-debug.txt --env-only --env-key env
tera-cli --template templates/env-debug.txt --env-only --env-key env

@@ -99,6 +118,10 @@ By default, your ENV variables will be loaded at the root of the context data. F

While the syntax is a little more verbose, paired with `--fail-on-collision`, this option allows ensuring that nothing happens in your back.

=== External files
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not push to your branch so you will have to update the mardown. This is just about running a single command.

The easiest if you have the tools is to run just md.

Alternatively, you may simply apply the following patch (assuming you do not modify the .adoc further):

diff --git a/README.md b/README.md
index fb0caea..e580470 100644
--- a/README.md
+++ b/README.md
@@ -75,6 +75,25 @@ The **tera** engine allows way more than the simple replacements shown above. Yo
 
     cargo install --git https://github.com/chevdor/tera-cli
 
+## Execute from container
+
+### Build container image
+
+    docker build --rm --tag 'tera/cli' .
+
+### Execute `tera` from container
+
+Check tera help
+
+    docker run --interactive --tty --rm tera/cli --help
+
+Parse a template
+
+    docker run --interactive --tty --rm \
+        --volume="$(pwd):/opt" \
+        --env=FOO=BAR \
+        tera/cli --template templates/env-debug.txt --env-only --env-key env
+
 ## What can I do with that anyway ?
 
 Well…​ if you have **data** and you want to format them, this tool will likely be a great companion.
@@ -125,6 +144,10 @@ By default, your ENV variables will be loaded at the root of the context data. F
 
 While the syntax is a little more verbose, paired with `--fail-on-collision`, this option allows ensuring that nothing happens in your back.
 
+### External files
+
+Using the `--include` flag, the command will scan recursively for files that could be [included](https://tera.netlify.app/docs/#include), used as [macros](https://tera.netlify.app/docs/#macros) or for [inheritance](https://tera.netlify.app/docs/#inheritance). By default, it will scan the folder where the main template is located, unless the `--include-path` option is given.
+
 ### Output
 
 By default,

@chevdor
Copy link
Owner

chevdor commented Sep 3, 2021

Here is a simple inheritance sample:

./data/inheritance/base.txt:

This is a BASE template.
The default greetings shows "Gretings !" but we can improve that with inheritance!
{% block greetings %}
Greetings !
{% endblock head %}

./data/inheritance/child.txt:

{% extends "base.txt" %}

The following block will replace our greetings in the base.
{% block greetings %}
May the force be with you ✨ {{ USER }} ✨
{% endblock greetings %}

And the call:

tera -t ./data/inheritance/child.txt --include --env-only

@@ -11,6 +11,15 @@ pub struct Opts {
#[clap(short, long)]
pub template: PathBuf,

/// This flag tells the command to parse all templates found in the same
/// path where the given template is located.
#[clap(short, long)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking the following may help:

Suggested change
#[clap(short, long)]
#[clap(short, long, alias="inherit")]

what do you think ?

That would make the inheritance example look like:

tera -t ./data/inheritance/child.txt --inherit --env-only

@tandiljuan
Copy link
Contributor Author

Hi @chevdor ! I'm going to close this pull request and split it in two new pull requests with the suggested changes.

@tandiljuan tandiljuan closed this Sep 3, 2021
@chevdor
Copy link
Owner

chevdor commented Sep 3, 2021

ok, thanks for the extra efforts :)
If you make them into branches and allow access, I can take care of the .adoc conversions for instance and directly push to your branch.

This was referenced Sep 5, 2021
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.

2 participants