Allow specifying variables in include

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes#545

What is the rationale for this request?

Users may want to create include file templates with variables that can be filled in, allowing reuse. However, they should only apply to each file. For example,

## {{ title }}
This is a level {{ level }} topic.

Allow users to specify variables in an include tag

What changes did you make? (Give an overview)

Allow users to specify Markbind variables in an include tag:

<include src=”foo.md”>
<span id="title"><span>New Title</span></span> // same syntax as variables.md
<span id="level">2</span>
</include>

These add on to (and overwrite) variables in _markbind/variables.md, and only apply to the context of the included file, and any files that the file includes.

We use nunjuck's or (default) syntax to allow for default values in case the variable is missing: {{ title or "Untitled" }}

Is there anything you'd like reviewers to focus on?

Users cannot reference other variables in the include, like this:

<include src=”foo.md”>
<span id="inner">Inner</span>
<span id="outer">Outer and {{ inner }}</span>
</include>

This is because {{ inner }} gets cleared when the original file is rendered. We can have a separate syntax for these, but it's a little unwieldly.

Referencing variables in _markbind/variables.md is okay.

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<spanid="color">red</span><fontcolor="{{ color }}">my text</font>

[jamos-tay]

@damithc Yes, tested and works

[damithc]

This PR affects the following statement, right?

[acjh]

Please use meaningful commit messages instead of "Pr fixes". 🙈

[jamos-tay]

Sorry =/

I'll do a quick rebase after it's approved

[yamgent]

1. Let's follow the same heading level as "<include> Inside an Included File".
2. Let's use "<include>" rather than "Include Tag" to follow the style of the rest of the documentation.

##### Specifying Variables in an `<include>`

[yamgent]

**It is possible to declare variables in an `<include>`.**

[yamgent]

... in an `<include>` tag:

[yamgent]

1. Fixing the grammar.
2. Rephrased the explanation of it being a template mechanism.

..., except that they only apply to the included file. They allow the included file to be reused as a template, for different source files using different variable values.

[damithc]

Side question: do they also work as nunjucks variables? e.g., {{ (size | int) - 1 }} would evaluate to 3 if <span id="size">4</span>

[jamos-tay]

Tested and it works - but do we want to document this? It seems like a small enough case, and it might confuse the user since it introduces another api

[damithc]

We already claim that MarkBind is generally compatible with nunjucks syntax. Here, we are using nunjucks syntax for a MarkBind feature. It's better to clarify that these variables are in fact equivalent to nunjucks variables declared within the scope of the included file. However, it can be mentioned in https://markbind.github.io/markbind/userGuide/markBindSyntax.html#support-for-nunjucks rather than under this feature. What do you think?

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<span id="color">red</span>
<font color="{{ color }}">my text</font>

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

[jamos-tay]

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

@damithc Sorry I think I misunderstood, were you referring to

<span id="color">red</span>
<font color="{{ color }}">my text</font>

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes so I put the span in variables.md

I don't think there's a way to create a local variable for a page just yet, but it probably should be in its own PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

[jamos-tay]

Updated

[damithc]

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes.

I don't think there's a way to create a local variable for a page just yet, but it should be put it in a different PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

I see. I thought we got that feature for free. Never mind then. It can be a feature to be added in the future.

[yamgent]

The wrong HTML tag is escaped, it didn't render correctly.

[yamgent]

Also it is not clear that the two different code exist in two different places, especially when you look at this portion in the live preview. Maybe indicate one is in variables.md and one is in index.md?

[yamgent]

Same problem (wrong HTML tag escaped).

[jamos-tay]

Updated, I decided to delete that section since there was a misunderstanding.

[yamgent]

1. The "Missing 'id'" part should actually be in front, in order to match the style of other error messages that we show as well.
2. I realise that the "Missing reference..." should be rephrased (because it is not a missing reference, but rather, a missing variable name), apologies about that.

So it should be something like this:

diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js
index 69c40e5..b71b50f 100644
--- a/src/lib/markbind/src/parser.js+++ b/src/lib/markbind/src/parser.js@@ -215,8 +215,8 @@ Parser.prototype._preprocess = function (node, context, config) {
}
if (!child.attribs.id) {
// eslint-disable-next-line no-console
- console.warn(`Missing reference in ${element.attribs[ATTRIB_CWF]}\n`- + `Missing 'id' in variable for ${element.attribs.src} include.`);+ console.warn(`Missing 'id' in variable for referenced file: ${element.attribs.src}.\n`+ + `Missing variable name in ${element.attribs[ATTRIB_CWF]}\n`);
return;
}
includedVariables[child.attribs.id] = cheerio.html(child.children);

[damithc]

@jamos-tay do look into how variables propagate via nested includes. Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

[yamgent]

Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

---

index.md

<includesrc="a.md"><spanid="someVariable">Original variable</span></include>

a.md

<includesrc="b.md"><spanid="someVariable">Nested include variable</span></include>

b.md

{{someVariable}}

---

[damithc]

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

Yes, that feels odd but from a reuse perspective that should be the behavior (opposite of how inheritance works). Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

[yamgent]

Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

That is indeed true, I think that is the desirable behaviour.

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"? Maybe "page variables" vs "global variables"? So we can say that "If a page uses a {{variable}}, then:

1. Page variables override global variables.
2. Outer page variables override inner page variables.

[damithc]

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"?

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

1. Page variables override global variables.

This should be the other way around too?

Maybe "page variables" vs "global variables"?

Or stay away from variables altogether. customization?

[yamgent]

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

Yes, we are using nunjucks behind the scene to implement variables.

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

This should be the other way around too?

Got it, global ones should have the highest priority. @jamos-tay you will have to rework this. :P

[damithc]

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

It would break the syntax introduced in this PR, right? :-) If the new syntax is introduced in this PR, it will not break existing features.
Anyway, it is a big change, and I'm not sure how hard it is to implement, especially if we cannot piggyback on nunjucks anymore. We can discuss on Saturday.

[jamos-tay]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

I'll need a way to detect whether a variable is a global or page variable though, but I think it can be done without using a hacky solution.

[damithc]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

Reusing the same syntax is appropriate only if a normal nunjucks variable behaves exactly the same way as the ones used for includes. Are we going to apply the same outermost-overrides behavior to normal nunjucks variables as well? i.e., those declared using {% set x = 'foo' %}

[jamos-tay]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

[damithc]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

Yup, this is a big decision. Better to make it now rather than later but we have to think it through. Although the issue came up in this PR, we are actually dealing with a much wider issue, namely, the need to support the outer-overrides-inner behavior that is central to reuse but opposite of the normal template behavior. The problem seem to be that we cannot support two opposing behaviors using the same internal library and/or syntax.

[jamos-tay]

After discussion:

We'll go with the original behavior:
Global variables override included variables
Outer include variables override inner include variables
{% set ... %} overrides all variables, but only applies to the page (excluding includes)

E.g.
variables.md: a = 1

a.md

<include src = b.md>
a = 2
b = 2
</include>

b.md

{{ a }} // 1
{{ b }} // 2
{% set a = 4 %}
{{ a }} // 4
<include src = c.md>
a = 3
b = 3
</include>

c.md

{{ a }} // 1
{{ b }} // 2

[jamos-tay]

Updated

Note: We currently do not support include variables referencing variables within the same include:

index.md:

<includesrc="a.md"><spanid="a">a</span><spanid="b">b & {{ a }}</span></include>

This is because {{ a }} gets rendered to an empty string when nunjucks processes index.md. Making this work would require some rework the page generation logic, which should be in another PR. This rework is also necessary to handle certain cases with set, so it can be fixed along with that.

[jamos-tay]

Rebased on master

[jamos-tay]

Updated

[yamgent]

Some more nits.

[jamos-tay]

Updated

[damithc]

Applied this in production nus-cs3281/2019@0d04ee4

Nice work @jamos-tay and reviewers @acjh@yamgent 👍

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes#545

What is the rationale for this request?

Users may want to create include file templates with variables that can be filled in, allowing reuse. However, they should only apply to each file. For example,

## {{ title }}
This is a level {{ level }} topic.

Allow users to specify variables in an include tag

What changes did you make? (Give an overview)

Allow users to specify Markbind variables in an include tag:

<include src=”foo.md”>
<span id="title"><span>New Title</span></span> // same syntax as variables.md
<span id="level">2</span>
</include>

These add on to (and overwrite) variables in _markbind/variables.md, and only apply to the context of the included file, and any files that the file includes.

We use nunjuck's or (default) syntax to allow for default values in case the variable is missing: {{ title or "Untitled" }}

Is there anything you'd like reviewers to focus on?

Users cannot reference other variables in the include, like this:

<include src=”foo.md”>
<span id="inner">Inner</span>
<span id="outer">Outer and {{ inner }}</span>
</include>

This is because {{ inner }} gets cleared when the original file is rendered. We can have a separate syntax for these, but it's a little unwieldly.

Referencing variables in _markbind/variables.md is okay.

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<spanid="color">red</span><fontcolor="{{ color }}">my text</font>

[jamos-tay]

@damithc Yes, tested and works

[damithc]

This PR affects the following statement, right?

[acjh]

Please use meaningful commit messages instead of "Pr fixes". 🙈

[jamos-tay]

Sorry =/

I'll do a quick rebase after it's approved

[yamgent]

1. Let's follow the same heading level as "<include> Inside an Included File".
2. Let's use "<include>" rather than "Include Tag" to follow the style of the rest of the documentation.

##### Specifying Variables in an `<include>`

[yamgent]

**It is possible to declare variables in an `<include>`.**

[yamgent]

... in an `<include>` tag:

[yamgent]

1. Fixing the grammar.
2. Rephrased the explanation of it being a template mechanism.

..., except that they only apply to the included file. They allow the included file to be reused as a template, for different source files using different variable values.

[damithc]

Side question: do they also work as nunjucks variables? e.g., {{ (size | int) - 1 }} would evaluate to 3 if <span id="size">4</span>

[jamos-tay]

Tested and it works - but do we want to document this? It seems like a small enough case, and it might confuse the user since it introduces another api

[damithc]

We already claim that MarkBind is generally compatible with nunjucks syntax. Here, we are using nunjucks syntax for a MarkBind feature. It's better to clarify that these variables are in fact equivalent to nunjucks variables declared within the scope of the included file. However, it can be mentioned in https://markbind.github.io/markbind/userGuide/markBindSyntax.html#support-for-nunjucks rather than under this feature. What do you think?

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<span id="color">red</span>
<font color="{{ color }}">my text</font>

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

[jamos-tay]

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

@damithc Sorry I think I misunderstood, were you referring to

<span id="color">red</span>
<font color="{{ color }}">my text</font>

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes so I put the span in variables.md

I don't think there's a way to create a local variable for a page just yet, but it probably should be in its own PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

[jamos-tay]

Updated

[damithc]

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes.

I don't think there's a way to create a local variable for a page just yet, but it should be put it in a different PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

I see. I thought we got that feature for free. Never mind then. It can be a feature to be added in the future.

[yamgent]

The wrong HTML tag is escaped, it didn't render correctly.

[yamgent]

Also it is not clear that the two different code exist in two different places, especially when you look at this portion in the live preview. Maybe indicate one is in variables.md and one is in index.md?

[yamgent]

Same problem (wrong HTML tag escaped).

[jamos-tay]

Updated, I decided to delete that section since there was a misunderstanding.

[yamgent]

1. The "Missing 'id'" part should actually be in front, in order to match the style of other error messages that we show as well.
2. I realise that the "Missing reference..." should be rephrased (because it is not a missing reference, but rather, a missing variable name), apologies about that.

So it should be something like this:

diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js
index 69c40e5..b71b50f 100644
--- a/src/lib/markbind/src/parser.js+++ b/src/lib/markbind/src/parser.js@@ -215,8 +215,8 @@ Parser.prototype._preprocess = function (node, context, config) {
}
if (!child.attribs.id) {
// eslint-disable-next-line no-console
- console.warn(`Missing reference in ${element.attribs[ATTRIB_CWF]}\n`- + `Missing 'id' in variable for ${element.attribs.src} include.`);+ console.warn(`Missing 'id' in variable for referenced file: ${element.attribs.src}.\n`+ + `Missing variable name in ${element.attribs[ATTRIB_CWF]}\n`);
return;
}
includedVariables[child.attribs.id] = cheerio.html(child.children);

[damithc]

@jamos-tay do look into how variables propagate via nested includes. Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

[yamgent]

Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

---

index.md

<includesrc="a.md"><spanid="someVariable">Original variable</span></include>

a.md

<includesrc="b.md"><spanid="someVariable">Nested include variable</span></include>

b.md

{{someVariable}}

---

[damithc]

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

Yes, that feels odd but from a reuse perspective that should be the behavior (opposite of how inheritance works). Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

[yamgent]

Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

That is indeed true, I think that is the desirable behaviour.

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"? Maybe "page variables" vs "global variables"? So we can say that "If a page uses a {{variable}}, then:

1. Page variables override global variables.
2. Outer page variables override inner page variables.

[damithc]

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"?

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

1. Page variables override global variables.

This should be the other way around too?

Maybe "page variables" vs "global variables"?

Or stay away from variables altogether. customization?

[yamgent]

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

Yes, we are using nunjucks behind the scene to implement variables.

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

This should be the other way around too?

Got it, global ones should have the highest priority. @jamos-tay you will have to rework this. :P

[damithc]

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

It would break the syntax introduced in this PR, right? :-) If the new syntax is introduced in this PR, it will not break existing features.
Anyway, it is a big change, and I'm not sure how hard it is to implement, especially if we cannot piggyback on nunjucks anymore. We can discuss on Saturday.

[jamos-tay]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

I'll need a way to detect whether a variable is a global or page variable though, but I think it can be done without using a hacky solution.

[damithc]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

Reusing the same syntax is appropriate only if a normal nunjucks variable behaves exactly the same way as the ones used for includes. Are we going to apply the same outermost-overrides behavior to normal nunjucks variables as well? i.e., those declared using {% set x = 'foo' %}

[jamos-tay]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

[damithc]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

Yup, this is a big decision. Better to make it now rather than later but we have to think it through. Although the issue came up in this PR, we are actually dealing with a much wider issue, namely, the need to support the outer-overrides-inner behavior that is central to reuse but opposite of the normal template behavior. The problem seem to be that we cannot support two opposing behaviors using the same internal library and/or syntax.

[jamos-tay]

After discussion:

We'll go with the original behavior:
Global variables override included variables
Outer include variables override inner include variables
{% set ... %} overrides all variables, but only applies to the page (excluding includes)

E.g.
variables.md: a = 1

a.md

<include src = b.md>
a = 2
b = 2
</include>

b.md

{{ a }} // 1
{{ b }} // 2
{% set a = 4 %}
{{ a }} // 4
<include src = c.md>
a = 3
b = 3
</include>

c.md

{{ a }} // 1
{{ b }} // 2

[jamos-tay]

Updated

Note: We currently do not support include variables referencing variables within the same include:

index.md:

<includesrc="a.md"><spanid="a">a</span><spanid="b">b & {{ a }}</span></include>

This is because {{ a }} gets rendered to an empty string when nunjucks processes index.md. Making this work would require some rework the page generation logic, which should be in another PR. This rework is also necessary to handle certain cases with set, so it can be fixed along with that.

[jamos-tay]

Rebased on master

[jamos-tay]

Updated

[yamgent]

Some more nits.

[jamos-tay]

Updated

[damithc]

Applied this in production nus-cs3281/2019@0d04ee4

Nice work @jamos-tay and reviewers @acjh@yamgent 👍

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes#545

What is the rationale for this request?

Users may want to create include file templates with variables that can be filled in, allowing reuse. However, they should only apply to each file. For example,

## {{ title }}
This is a level {{ level }} topic.

Allow users to specify variables in an include tag

What changes did you make? (Give an overview)

Allow users to specify Markbind variables in an include tag:

<include src=”foo.md”>
<span id="title"><span>New Title</span></span> // same syntax as variables.md
<span id="level">2</span>
</include>

These add on to (and overwrite) variables in _markbind/variables.md, and only apply to the context of the included file, and any files that the file includes.

We use nunjuck's or (default) syntax to allow for default values in case the variable is missing: {{ title or "Untitled" }}

Is there anything you'd like reviewers to focus on?

Users cannot reference other variables in the include, like this:

<include src=”foo.md”>
<span id="inner">Inner</span>
<span id="outer">Outer and {{ inner }}</span>
</include>

This is because {{ inner }} gets cleared when the original file is rendered. We can have a separate syntax for these, but it's a little unwieldly.

Referencing variables in _markbind/variables.md is okay.

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<spanid="color">red</span><fontcolor="{{ color }}">my text</font>

[jamos-tay]

@damithc Yes, tested and works

[damithc]

This PR affects the following statement, right?

[acjh]

Please use meaningful commit messages instead of "Pr fixes". 🙈

[jamos-tay]

Sorry =/

I'll do a quick rebase after it's approved

[yamgent]

1. Let's follow the same heading level as "<include> Inside an Included File".
2. Let's use "<include>" rather than "Include Tag" to follow the style of the rest of the documentation.

##### Specifying Variables in an `<include>`

[yamgent]

**It is possible to declare variables in an `<include>`.**

[yamgent]

... in an `<include>` tag:

[yamgent]

1. Fixing the grammar.
2. Rephrased the explanation of it being a template mechanism.

..., except that they only apply to the included file. They allow the included file to be reused as a template, for different source files using different variable values.

[damithc]

Side question: do they also work as nunjucks variables? e.g., {{ (size | int) - 1 }} would evaluate to 3 if <span id="size">4</span>

[jamos-tay]

Tested and it works - but do we want to document this? It seems like a small enough case, and it might confuse the user since it introduces another api

[damithc]

We already claim that MarkBind is generally compatible with nunjucks syntax. Here, we are using nunjucks syntax for a MarkBind feature. It's better to clarify that these variables are in fact equivalent to nunjucks variables declared within the scope of the included file. However, it can be mentioned in https://markbind.github.io/markbind/userGuide/markBindSyntax.html#support-for-nunjucks rather than under this feature. What do you think?

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<span id="color">red</span>
<font color="{{ color }}">my text</font>

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

[jamos-tay]

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

@damithc Sorry I think I misunderstood, were you referring to

<span id="color">red</span>
<font color="{{ color }}">my text</font>

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes so I put the span in variables.md

I don't think there's a way to create a local variable for a page just yet, but it probably should be in its own PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

[jamos-tay]

Updated

[damithc]

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes.

I don't think there's a way to create a local variable for a page just yet, but it should be put it in a different PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

I see. I thought we got that feature for free. Never mind then. It can be a feature to be added in the future.

[yamgent]

The wrong HTML tag is escaped, it didn't render correctly.

[yamgent]

Also it is not clear that the two different code exist in two different places, especially when you look at this portion in the live preview. Maybe indicate one is in variables.md and one is in index.md?

[yamgent]

Same problem (wrong HTML tag escaped).

[jamos-tay]

Updated, I decided to delete that section since there was a misunderstanding.

[yamgent]

1. The "Missing 'id'" part should actually be in front, in order to match the style of other error messages that we show as well.
2. I realise that the "Missing reference..." should be rephrased (because it is not a missing reference, but rather, a missing variable name), apologies about that.

So it should be something like this:

diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js
index 69c40e5..b71b50f 100644
--- a/src/lib/markbind/src/parser.js+++ b/src/lib/markbind/src/parser.js@@ -215,8 +215,8 @@ Parser.prototype._preprocess = function (node, context, config) {
}
if (!child.attribs.id) {
// eslint-disable-next-line no-console
- console.warn(`Missing reference in ${element.attribs[ATTRIB_CWF]}\n`- + `Missing 'id' in variable for ${element.attribs.src} include.`);+ console.warn(`Missing 'id' in variable for referenced file: ${element.attribs.src}.\n`+ + `Missing variable name in ${element.attribs[ATTRIB_CWF]}\n`);
return;
}
includedVariables[child.attribs.id] = cheerio.html(child.children);

[damithc]

@jamos-tay do look into how variables propagate via nested includes. Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

[yamgent]

Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

---

index.md

<includesrc="a.md"><spanid="someVariable">Original variable</span></include>

a.md

<includesrc="b.md"><spanid="someVariable">Nested include variable</span></include>

b.md

{{someVariable}}

---

[damithc]

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

Yes, that feels odd but from a reuse perspective that should be the behavior (opposite of how inheritance works). Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

[yamgent]

Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

That is indeed true, I think that is the desirable behaviour.

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"? Maybe "page variables" vs "global variables"? So we can say that "If a page uses a {{variable}}, then:

1. Page variables override global variables.
2. Outer page variables override inner page variables.

[damithc]

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"?

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

1. Page variables override global variables.

This should be the other way around too?

Maybe "page variables" vs "global variables"?

Or stay away from variables altogether. customization?

[yamgent]

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

Yes, we are using nunjucks behind the scene to implement variables.

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

This should be the other way around too?

Got it, global ones should have the highest priority. @jamos-tay you will have to rework this. :P

[damithc]

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

It would break the syntax introduced in this PR, right? :-) If the new syntax is introduced in this PR, it will not break existing features.
Anyway, it is a big change, and I'm not sure how hard it is to implement, especially if we cannot piggyback on nunjucks anymore. We can discuss on Saturday.

[jamos-tay]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

I'll need a way to detect whether a variable is a global or page variable though, but I think it can be done without using a hacky solution.

[damithc]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

Reusing the same syntax is appropriate only if a normal nunjucks variable behaves exactly the same way as the ones used for includes. Are we going to apply the same outermost-overrides behavior to normal nunjucks variables as well? i.e., those declared using {% set x = 'foo' %}

[jamos-tay]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

[damithc]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

Yup, this is a big decision. Better to make it now rather than later but we have to think it through. Although the issue came up in this PR, we are actually dealing with a much wider issue, namely, the need to support the outer-overrides-inner behavior that is central to reuse but opposite of the normal template behavior. The problem seem to be that we cannot support two opposing behaviors using the same internal library and/or syntax.

[jamos-tay]

After discussion:

We'll go with the original behavior:
Global variables override included variables
Outer include variables override inner include variables
{% set ... %} overrides all variables, but only applies to the page (excluding includes)

E.g.
variables.md: a = 1

a.md

<include src = b.md>
a = 2
b = 2
</include>

b.md

{{ a }} // 1
{{ b }} // 2
{% set a = 4 %}
{{ a }} // 4
<include src = c.md>
a = 3
b = 3
</include>

c.md

{{ a }} // 1
{{ b }} // 2

[jamos-tay]

Updated

Note: We currently do not support include variables referencing variables within the same include:

index.md:

<includesrc="a.md"><spanid="a">a</span><spanid="b">b & {{ a }}</span></include>

This is because {{ a }} gets rendered to an empty string when nunjucks processes index.md. Making this work would require some rework the page generation logic, which should be in another PR. This rework is also necessary to handle certain cases with set, so it can be fixed along with that.

[jamos-tay]

Rebased on master

[jamos-tay]

Updated

[yamgent]

Some more nits.

[jamos-tay]

Updated

[damithc]

Applied this in production nus-cs3281/2019@0d04ee4

Nice work @jamos-tay and reviewers @acjh@yamgent 👍

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes#545

What is the rationale for this request?

Users may want to create include file templates with variables that can be filled in, allowing reuse. However, they should only apply to each file. For example,

## {{ title }}
This is a level {{ level }} topic.

Allow users to specify variables in an include tag

What changes did you make? (Give an overview)

Allow users to specify Markbind variables in an include tag:

<include src=”foo.md”>
<span id="title"><span>New Title</span></span> // same syntax as variables.md
<span id="level">2</span>
</include>

These add on to (and overwrite) variables in _markbind/variables.md, and only apply to the context of the included file, and any files that the file includes.

We use nunjuck's or (default) syntax to allow for default values in case the variable is missing: {{ title or "Untitled" }}

Is there anything you'd like reviewers to focus on?

Users cannot reference other variables in the include, like this:

<include src=”foo.md”>
<span id="inner">Inner</span>
<span id="outer">Outer and {{ inner }}</span>
</include>

This is because {{ inner }} gets cleared when the original file is rendered. We can have a separate syntax for these, but it's a little unwieldly.

Referencing variables in _markbind/variables.md is okay.

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<spanid="color">red</span><fontcolor="{{ color }}">my text</font>

[jamos-tay]

@damithc Yes, tested and works

[damithc]

This PR affects the following statement, right?

[acjh]

Please use meaningful commit messages instead of "Pr fixes". 🙈

[jamos-tay]

Sorry =/

I'll do a quick rebase after it's approved

[yamgent]

1. Let's follow the same heading level as "<include> Inside an Included File".
2. Let's use "<include>" rather than "Include Tag" to follow the style of the rest of the documentation.

##### Specifying Variables in an `<include>`

[yamgent]

**It is possible to declare variables in an `<include>`.**

[yamgent]

... in an `<include>` tag:

[yamgent]

1. Fixing the grammar.
2. Rephrased the explanation of it being a template mechanism.

..., except that they only apply to the included file. They allow the included file to be reused as a template, for different source files using different variable values.

[damithc]

Side question: do they also work as nunjucks variables? e.g., {{ (size | int) - 1 }} would evaluate to 3 if <span id="size">4</span>

[jamos-tay]

Tested and it works - but do we want to document this? It seems like a small enough case, and it might confuse the user since it introduces another api

[damithc]

We already claim that MarkBind is generally compatible with nunjucks syntax. Here, we are using nunjucks syntax for a MarkBind feature. It's better to clarify that these variables are in fact equivalent to nunjucks variables declared within the scope of the included file. However, it can be mentioned in https://markbind.github.io/markbind/userGuide/markBindSyntax.html#support-for-nunjucks rather than under this feature. What do you think?

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<span id="color">red</span>
<font color="{{ color }}">my text</font>

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

[jamos-tay]

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

@damithc Sorry I think I misunderstood, were you referring to

<span id="color">red</span>
<font color="{{ color }}">my text</font>

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes so I put the span in variables.md

I don't think there's a way to create a local variable for a page just yet, but it probably should be in its own PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

[jamos-tay]

Updated

[damithc]

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes.

I don't think there's a way to create a local variable for a page just yet, but it should be put it in a different PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

I see. I thought we got that feature for free. Never mind then. It can be a feature to be added in the future.

[yamgent]

The wrong HTML tag is escaped, it didn't render correctly.

[yamgent]

Also it is not clear that the two different code exist in two different places, especially when you look at this portion in the live preview. Maybe indicate one is in variables.md and one is in index.md?

[yamgent]

Same problem (wrong HTML tag escaped).

[jamos-tay]

Updated, I decided to delete that section since there was a misunderstanding.

[yamgent]

1. The "Missing 'id'" part should actually be in front, in order to match the style of other error messages that we show as well.
2. I realise that the "Missing reference..." should be rephrased (because it is not a missing reference, but rather, a missing variable name), apologies about that.

So it should be something like this:

diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js
index 69c40e5..b71b50f 100644
--- a/src/lib/markbind/src/parser.js+++ b/src/lib/markbind/src/parser.js@@ -215,8 +215,8 @@ Parser.prototype._preprocess = function (node, context, config) {
}
if (!child.attribs.id) {
// eslint-disable-next-line no-console
- console.warn(`Missing reference in ${element.attribs[ATTRIB_CWF]}\n`- + `Missing 'id' in variable for ${element.attribs.src} include.`);+ console.warn(`Missing 'id' in variable for referenced file: ${element.attribs.src}.\n`+ + `Missing variable name in ${element.attribs[ATTRIB_CWF]}\n`);
return;
}
includedVariables[child.attribs.id] = cheerio.html(child.children);

[damithc]

@jamos-tay do look into how variables propagate via nested includes. Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

[yamgent]

Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

---

index.md

<includesrc="a.md"><spanid="someVariable">Original variable</span></include>

a.md

<includesrc="b.md"><spanid="someVariable">Nested include variable</span></include>

b.md

{{someVariable}}

---

[damithc]

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

Yes, that feels odd but from a reuse perspective that should be the behavior (opposite of how inheritance works). Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

[yamgent]

Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

That is indeed true, I think that is the desirable behaviour.

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"? Maybe "page variables" vs "global variables"? So we can say that "If a page uses a {{variable}}, then:

1. Page variables override global variables.
2. Outer page variables override inner page variables.

[damithc]

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"?

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

1. Page variables override global variables.

This should be the other way around too?

Maybe "page variables" vs "global variables"?

Or stay away from variables altogether. customization?

[yamgent]

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

Yes, we are using nunjucks behind the scene to implement variables.

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

This should be the other way around too?

Got it, global ones should have the highest priority. @jamos-tay you will have to rework this. :P

[damithc]

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

It would break the syntax introduced in this PR, right? :-) If the new syntax is introduced in this PR, it will not break existing features.
Anyway, it is a big change, and I'm not sure how hard it is to implement, especially if we cannot piggyback on nunjucks anymore. We can discuss on Saturday.

[jamos-tay]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

I'll need a way to detect whether a variable is a global or page variable though, but I think it can be done without using a hacky solution.

[damithc]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

Reusing the same syntax is appropriate only if a normal nunjucks variable behaves exactly the same way as the ones used for includes. Are we going to apply the same outermost-overrides behavior to normal nunjucks variables as well? i.e., those declared using {% set x = 'foo' %}

[jamos-tay]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

[damithc]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

Yup, this is a big decision. Better to make it now rather than later but we have to think it through. Although the issue came up in this PR, we are actually dealing with a much wider issue, namely, the need to support the outer-overrides-inner behavior that is central to reuse but opposite of the normal template behavior. The problem seem to be that we cannot support two opposing behaviors using the same internal library and/or syntax.

[jamos-tay]

After discussion:

We'll go with the original behavior:
Global variables override included variables
Outer include variables override inner include variables
{% set ... %} overrides all variables, but only applies to the page (excluding includes)

E.g.
variables.md: a = 1

a.md

<include src = b.md>
a = 2
b = 2
</include>

b.md

{{ a }} // 1
{{ b }} // 2
{% set a = 4 %}
{{ a }} // 4
<include src = c.md>
a = 3
b = 3
</include>

c.md

{{ a }} // 1
{{ b }} // 2

[jamos-tay]

Updated

Note: We currently do not support include variables referencing variables within the same include:

index.md:

<includesrc="a.md"><spanid="a">a</span><spanid="b">b & {{ a }}</span></include>

This is because {{ a }} gets rendered to an empty string when nunjucks processes index.md. Making this work would require some rework the page generation logic, which should be in another PR. This rework is also necessary to handle certain cases with set, so it can be fixed along with that.

[jamos-tay]

Rebased on master

[jamos-tay]

Updated

[yamgent]

Some more nits.

[jamos-tay]

Updated

[damithc]

Applied this in production nus-cs3281/2019@0d04ee4

Nice work @jamos-tay and reviewers @acjh@yamgent 👍

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes#545

What is the rationale for this request?

Users may want to create include file templates with variables that can be filled in, allowing reuse. However, they should only apply to each file. For example,

## {{ title }}
This is a level {{ level }} topic.

Allow users to specify variables in an include tag

What changes did you make? (Give an overview)

Allow users to specify Markbind variables in an include tag:

<include src=”foo.md”>
<span id="title"><span>New Title</span></span> // same syntax as variables.md
<span id="level">2</span>
</include>

These add on to (and overwrite) variables in _markbind/variables.md, and only apply to the context of the included file, and any files that the file includes.

We use nunjuck's or (default) syntax to allow for default values in case the variable is missing: {{ title or "Untitled" }}

Is there anything you'd like reviewers to focus on?

Users cannot reference other variables in the include, like this:

<include src=”foo.md”>
<span id="inner">Inner</span>
<span id="outer">Outer and {{ inner }}</span>
</include>

This is because {{ inner }} gets cleared when the original file is rendered. We can have a separate syntax for these, but it's a little unwieldly.

Referencing variables in _markbind/variables.md is okay.

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<spanid="color">red</span><fontcolor="{{ color }}">my text</font>

[jamos-tay]

@damithc Yes, tested and works

[damithc]

This PR affects the following statement, right?

[acjh]

Please use meaningful commit messages instead of "Pr fixes". 🙈

[jamos-tay]

Sorry =/

I'll do a quick rebase after it's approved

[yamgent]

1. Let's follow the same heading level as "<include> Inside an Included File".
2. Let's use "<include>" rather than "Include Tag" to follow the style of the rest of the documentation.

##### Specifying Variables in an `<include>`

[yamgent]

**It is possible to declare variables in an `<include>`.**

[yamgent]

... in an `<include>` tag:

[yamgent]

1. Fixing the grammar.
2. Rephrased the explanation of it being a template mechanism.

..., except that they only apply to the included file. They allow the included file to be reused as a template, for different source files using different variable values.

[damithc]

Side question: do they also work as nunjucks variables? e.g., {{ (size | int) - 1 }} would evaluate to 3 if <span id="size">4</span>

[jamos-tay]

Tested and it works - but do we want to document this? It seems like a small enough case, and it might confuse the user since it introduces another api

[damithc]

We already claim that MarkBind is generally compatible with nunjucks syntax. Here, we are using nunjucks syntax for a MarkBind feature. It's better to clarify that these variables are in fact equivalent to nunjucks variables declared within the scope of the included file. However, it can be mentioned in https://markbind.github.io/markbind/userGuide/markBindSyntax.html#support-for-nunjucks rather than under this feature. What do you think?

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<span id="color">red</span>
<font color="{{ color }}">my text</font>

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

[jamos-tay]

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

@damithc Sorry I think I misunderstood, were you referring to

<span id="color">red</span>
<font color="{{ color }}">my text</font>

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes so I put the span in variables.md

I don't think there's a way to create a local variable for a page just yet, but it probably should be in its own PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

[jamos-tay]

Updated

[damithc]

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes.

I don't think there's a way to create a local variable for a page just yet, but it should be put it in a different PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

I see. I thought we got that feature for free. Never mind then. It can be a feature to be added in the future.

[yamgent]

The wrong HTML tag is escaped, it didn't render correctly.

[yamgent]

Also it is not clear that the two different code exist in two different places, especially when you look at this portion in the live preview. Maybe indicate one is in variables.md and one is in index.md?

[yamgent]

Same problem (wrong HTML tag escaped).

[jamos-tay]

Updated, I decided to delete that section since there was a misunderstanding.

[yamgent]

1. The "Missing 'id'" part should actually be in front, in order to match the style of other error messages that we show as well.
2. I realise that the "Missing reference..." should be rephrased (because it is not a missing reference, but rather, a missing variable name), apologies about that.

So it should be something like this:

diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js
index 69c40e5..b71b50f 100644
--- a/src/lib/markbind/src/parser.js+++ b/src/lib/markbind/src/parser.js@@ -215,8 +215,8 @@ Parser.prototype._preprocess = function (node, context, config) {
}
if (!child.attribs.id) {
// eslint-disable-next-line no-console
- console.warn(`Missing reference in ${element.attribs[ATTRIB_CWF]}\n`- + `Missing 'id' in variable for ${element.attribs.src} include.`);+ console.warn(`Missing 'id' in variable for referenced file: ${element.attribs.src}.\n`+ + `Missing variable name in ${element.attribs[ATTRIB_CWF]}\n`);
return;
}
includedVariables[child.attribs.id] = cheerio.html(child.children);

[damithc]

@jamos-tay do look into how variables propagate via nested includes. Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

[yamgent]

Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

---

index.md

<includesrc="a.md"><spanid="someVariable">Original variable</span></include>

a.md

<includesrc="b.md"><spanid="someVariable">Nested include variable</span></include>

b.md

{{someVariable}}

---

[damithc]

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

Yes, that feels odd but from a reuse perspective that should be the behavior (opposite of how inheritance works). Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

[yamgent]

Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

That is indeed true, I think that is the desirable behaviour.

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"? Maybe "page variables" vs "global variables"? So we can say that "If a page uses a {{variable}}, then:

1. Page variables override global variables.
2. Outer page variables override inner page variables.

[damithc]

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"?

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

1. Page variables override global variables.

This should be the other way around too?

Maybe "page variables" vs "global variables"?

Or stay away from variables altogether. customization?

[yamgent]

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

Yes, we are using nunjucks behind the scene to implement variables.

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

This should be the other way around too?

Got it, global ones should have the highest priority. @jamos-tay you will have to rework this. :P

[damithc]

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

It would break the syntax introduced in this PR, right? :-) If the new syntax is introduced in this PR, it will not break existing features.
Anyway, it is a big change, and I'm not sure how hard it is to implement, especially if we cannot piggyback on nunjucks anymore. We can discuss on Saturday.

[jamos-tay]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

I'll need a way to detect whether a variable is a global or page variable though, but I think it can be done without using a hacky solution.

[damithc]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

Reusing the same syntax is appropriate only if a normal nunjucks variable behaves exactly the same way as the ones used for includes. Are we going to apply the same outermost-overrides behavior to normal nunjucks variables as well? i.e., those declared using {% set x = 'foo' %}

[jamos-tay]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

[damithc]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

Yup, this is a big decision. Better to make it now rather than later but we have to think it through. Although the issue came up in this PR, we are actually dealing with a much wider issue, namely, the need to support the outer-overrides-inner behavior that is central to reuse but opposite of the normal template behavior. The problem seem to be that we cannot support two opposing behaviors using the same internal library and/or syntax.

[jamos-tay]

After discussion:

We'll go with the original behavior:
Global variables override included variables
Outer include variables override inner include variables
{% set ... %} overrides all variables, but only applies to the page (excluding includes)

E.g.
variables.md: a = 1

a.md

<include src = b.md>
a = 2
b = 2
</include>

b.md

{{ a }} // 1
{{ b }} // 2
{% set a = 4 %}
{{ a }} // 4
<include src = c.md>
a = 3
b = 3
</include>

c.md

{{ a }} // 1
{{ b }} // 2

[jamos-tay]

Updated

Note: We currently do not support include variables referencing variables within the same include:

index.md:

<includesrc="a.md"><spanid="a">a</span><spanid="b">b & {{ a }}</span></include>

This is because {{ a }} gets rendered to an empty string when nunjucks processes index.md. Making this work would require some rework the page generation logic, which should be in another PR. This rework is also necessary to handle certain cases with set, so it can be fixed along with that.

[jamos-tay]

Rebased on master

[jamos-tay]

Updated

[yamgent]

Some more nits.

[jamos-tay]

Updated

[damithc]

Applied this in production nus-cs3281/2019@0d04ee4

Nice work @jamos-tay and reviewers @acjh@yamgent 👍

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes#545

What is the rationale for this request?

Users may want to create include file templates with variables that can be filled in, allowing reuse. However, they should only apply to each file. For example,

## {{ title }}
This is a level {{ level }} topic.

Allow users to specify variables in an include tag

What changes did you make? (Give an overview)

Allow users to specify Markbind variables in an include tag:

<include src=”foo.md”>
<span id="title"><span>New Title</span></span> // same syntax as variables.md
<span id="level">2</span>
</include>

These add on to (and overwrite) variables in _markbind/variables.md, and only apply to the context of the included file, and any files that the file includes.

We use nunjuck's or (default) syntax to allow for default values in case the variable is missing: {{ title or "Untitled" }}

Is there anything you'd like reviewers to focus on?

Users cannot reference other variables in the include, like this:

<include src=”foo.md”>
<span id="inner">Inner</span>
<span id="outer">Outer and {{ inner }}</span>
</include>

This is because {{ inner }} gets cleared when the original file is rendered. We can have a separate syntax for these, but it's a little unwieldly.

Referencing variables in _markbind/variables.md is okay.

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<spanid="color">red</span><fontcolor="{{ color }}">my text</font>

[jamos-tay]

@damithc Yes, tested and works

[damithc]

This PR affects the following statement, right?

[acjh]

Please use meaningful commit messages instead of "Pr fixes". 🙈

[jamos-tay]

Sorry =/

I'll do a quick rebase after it's approved

[yamgent]

1. Let's follow the same heading level as "<include> Inside an Included File".
2. Let's use "<include>" rather than "Include Tag" to follow the style of the rest of the documentation.

##### Specifying Variables in an `<include>`

[yamgent]

**It is possible to declare variables in an `<include>`.**

[yamgent]

... in an `<include>` tag:

[yamgent]

1. Fixing the grammar.
2. Rephrased the explanation of it being a template mechanism.

..., except that they only apply to the included file. They allow the included file to be reused as a template, for different source files using different variable values.

[damithc]

Side question: do they also work as nunjucks variables? e.g., {{ (size | int) - 1 }} would evaluate to 3 if <span id="size">4</span>

[jamos-tay]

Tested and it works - but do we want to document this? It seems like a small enough case, and it might confuse the user since it introduces another api

[damithc]

We already claim that MarkBind is generally compatible with nunjucks syntax. Here, we are using nunjucks syntax for a MarkBind feature. It's better to clarify that these variables are in fact equivalent to nunjucks variables declared within the scope of the included file. However, it can be mentioned in https://markbind.github.io/markbind/userGuide/markBindSyntax.html#support-for-nunjucks rather than under this feature. What do you think?

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<span id="color">red</span>
<font color="{{ color }}">my text</font>

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

[jamos-tay]

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

@damithc Sorry I think I misunderstood, were you referring to

<span id="color">red</span>
<font color="{{ color }}">my text</font>

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes so I put the span in variables.md

I don't think there's a way to create a local variable for a page just yet, but it probably should be in its own PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

[jamos-tay]

Updated

[damithc]

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes.

I don't think there's a way to create a local variable for a page just yet, but it should be put it in a different PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

I see. I thought we got that feature for free. Never mind then. It can be a feature to be added in the future.

[yamgent]

The wrong HTML tag is escaped, it didn't render correctly.

[yamgent]

Also it is not clear that the two different code exist in two different places, especially when you look at this portion in the live preview. Maybe indicate one is in variables.md and one is in index.md?

[yamgent]

Same problem (wrong HTML tag escaped).

[jamos-tay]

Updated, I decided to delete that section since there was a misunderstanding.

[yamgent]

1. The "Missing 'id'" part should actually be in front, in order to match the style of other error messages that we show as well.
2. I realise that the "Missing reference..." should be rephrased (because it is not a missing reference, but rather, a missing variable name), apologies about that.

So it should be something like this:

diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js
index 69c40e5..b71b50f 100644
--- a/src/lib/markbind/src/parser.js+++ b/src/lib/markbind/src/parser.js@@ -215,8 +215,8 @@ Parser.prototype._preprocess = function (node, context, config) {
}
if (!child.attribs.id) {
// eslint-disable-next-line no-console
- console.warn(`Missing reference in ${element.attribs[ATTRIB_CWF]}\n`- + `Missing 'id' in variable for ${element.attribs.src} include.`);+ console.warn(`Missing 'id' in variable for referenced file: ${element.attribs.src}.\n`+ + `Missing variable name in ${element.attribs[ATTRIB_CWF]}\n`);
return;
}
includedVariables[child.attribs.id] = cheerio.html(child.children);

[damithc]

@jamos-tay do look into how variables propagate via nested includes. Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

[yamgent]

Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

---

index.md

<includesrc="a.md"><spanid="someVariable">Original variable</span></include>

a.md

<includesrc="b.md"><spanid="someVariable">Nested include variable</span></include>

b.md

{{someVariable}}

---

[damithc]

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

Yes, that feels odd but from a reuse perspective that should be the behavior (opposite of how inheritance works). Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

[yamgent]

Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

That is indeed true, I think that is the desirable behaviour.

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"? Maybe "page variables" vs "global variables"? So we can say that "If a page uses a {{variable}}, then:

1. Page variables override global variables.
2. Outer page variables override inner page variables.

[damithc]

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"?

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

1. Page variables override global variables.

This should be the other way around too?

Maybe "page variables" vs "global variables"?

Or stay away from variables altogether. customization?

[yamgent]

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

Yes, we are using nunjucks behind the scene to implement variables.

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

This should be the other way around too?

Got it, global ones should have the highest priority. @jamos-tay you will have to rework this. :P

[damithc]

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

It would break the syntax introduced in this PR, right? :-) If the new syntax is introduced in this PR, it will not break existing features.
Anyway, it is a big change, and I'm not sure how hard it is to implement, especially if we cannot piggyback on nunjucks anymore. We can discuss on Saturday.

[jamos-tay]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

I'll need a way to detect whether a variable is a global or page variable though, but I think it can be done without using a hacky solution.

[damithc]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

Reusing the same syntax is appropriate only if a normal nunjucks variable behaves exactly the same way as the ones used for includes. Are we going to apply the same outermost-overrides behavior to normal nunjucks variables as well? i.e., those declared using {% set x = 'foo' %}

[jamos-tay]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

[damithc]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

Yup, this is a big decision. Better to make it now rather than later but we have to think it through. Although the issue came up in this PR, we are actually dealing with a much wider issue, namely, the need to support the outer-overrides-inner behavior that is central to reuse but opposite of the normal template behavior. The problem seem to be that we cannot support two opposing behaviors using the same internal library and/or syntax.

[jamos-tay]

After discussion:

We'll go with the original behavior:
Global variables override included variables
Outer include variables override inner include variables
{% set ... %} overrides all variables, but only applies to the page (excluding includes)

E.g.
variables.md: a = 1

a.md

<include src = b.md>
a = 2
b = 2
</include>

b.md

{{ a }} // 1
{{ b }} // 2
{% set a = 4 %}
{{ a }} // 4
<include src = c.md>
a = 3
b = 3
</include>

c.md

{{ a }} // 1
{{ b }} // 2

[jamos-tay]

Updated

Note: We currently do not support include variables referencing variables within the same include:

index.md:

<includesrc="a.md"><spanid="a">a</span><spanid="b">b & {{ a }}</span></include>

This is because {{ a }} gets rendered to an empty string when nunjucks processes index.md. Making this work would require some rework the page generation logic, which should be in another PR. This rework is also necessary to handle certain cases with set, so it can be fixed along with that.

[jamos-tay]

Rebased on master

[jamos-tay]

Updated

[yamgent]

Some more nits.

[jamos-tay]

Updated

[damithc]

Applied this in production nus-cs3281/2019@0d04ee4

Nice work @jamos-tay and reviewers @acjh@yamgent 👍

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes#545

What is the rationale for this request?

Users may want to create include file templates with variables that can be filled in, allowing reuse. However, they should only apply to each file. For example,

## {{ title }}
This is a level {{ level }} topic.

Allow users to specify variables in an include tag

What changes did you make? (Give an overview)

Allow users to specify Markbind variables in an include tag:

<include src=”foo.md”>
<span id="title"><span>New Title</span></span> // same syntax as variables.md
<span id="level">2</span>
</include>

These add on to (and overwrite) variables in _markbind/variables.md, and only apply to the context of the included file, and any files that the file includes.

We use nunjuck's or (default) syntax to allow for default values in case the variable is missing: {{ title or "Untitled" }}

Is there anything you'd like reviewers to focus on?

Users cannot reference other variables in the include, like this:

<include src=”foo.md”>
<span id="inner">Inner</span>
<span id="outer">Outer and {{ inner }}</span>
</include>

This is because {{ inner }} gets cleared when the original file is rendered. We can have a separate syntax for these, but it's a little unwieldly.

Referencing variables in _markbind/variables.md is okay.

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<spanid="color">red</span><fontcolor="{{ color }}">my text</font>

[jamos-tay]

@damithc Yes, tested and works

[damithc]

This PR affects the following statement, right?

[acjh]

Please use meaningful commit messages instead of "Pr fixes". 🙈

[jamos-tay]

Sorry =/

I'll do a quick rebase after it's approved

[yamgent]

1. Let's follow the same heading level as "<include> Inside an Included File".
2. Let's use "<include>" rather than "Include Tag" to follow the style of the rest of the documentation.

##### Specifying Variables in an `<include>`

[yamgent]

**It is possible to declare variables in an `<include>`.**

[yamgent]

... in an `<include>` tag:

[yamgent]

1. Fixing the grammar.
2. Rephrased the explanation of it being a template mechanism.

..., except that they only apply to the included file. They allow the included file to be reused as a template, for different source files using different variable values.

[damithc]

Side question: do they also work as nunjucks variables? e.g., {{ (size | int) - 1 }} would evaluate to 3 if <span id="size">4</span>

[jamos-tay]

Tested and it works - but do we want to document this? It seems like a small enough case, and it might confuse the user since it introduces another api

[damithc]

We already claim that MarkBind is generally compatible with nunjucks syntax. Here, we are using nunjucks syntax for a MarkBind feature. It's better to clarify that these variables are in fact equivalent to nunjucks variables declared within the scope of the included file. However, it can be mentioned in https://markbind.github.io/markbind/userGuide/markBindSyntax.html#support-for-nunjucks rather than under this feature. What do you think?

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<span id="color">red</span>
<font color="{{ color }}">my text</font>

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

[jamos-tay]

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

@damithc Sorry I think I misunderstood, were you referring to

<span id="color">red</span>
<font color="{{ color }}">my text</font>

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes so I put the span in variables.md

I don't think there's a way to create a local variable for a page just yet, but it probably should be in its own PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

[jamos-tay]

Updated

[damithc]

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes.

I don't think there's a way to create a local variable for a page just yet, but it should be put it in a different PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

I see. I thought we got that feature for free. Never mind then. It can be a feature to be added in the future.

[yamgent]

The wrong HTML tag is escaped, it didn't render correctly.

[yamgent]

Also it is not clear that the two different code exist in two different places, especially when you look at this portion in the live preview. Maybe indicate one is in variables.md and one is in index.md?

[yamgent]

Same problem (wrong HTML tag escaped).

[jamos-tay]

Updated, I decided to delete that section since there was a misunderstanding.

[yamgent]

1. The "Missing 'id'" part should actually be in front, in order to match the style of other error messages that we show as well.
2. I realise that the "Missing reference..." should be rephrased (because it is not a missing reference, but rather, a missing variable name), apologies about that.

So it should be something like this:

diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js
index 69c40e5..b71b50f 100644
--- a/src/lib/markbind/src/parser.js+++ b/src/lib/markbind/src/parser.js@@ -215,8 +215,8 @@ Parser.prototype._preprocess = function (node, context, config) {
}
if (!child.attribs.id) {
// eslint-disable-next-line no-console
- console.warn(`Missing reference in ${element.attribs[ATTRIB_CWF]}\n`- + `Missing 'id' in variable for ${element.attribs.src} include.`);+ console.warn(`Missing 'id' in variable for referenced file: ${element.attribs.src}.\n`+ + `Missing variable name in ${element.attribs[ATTRIB_CWF]}\n`);
return;
}
includedVariables[child.attribs.id] = cheerio.html(child.children);

[damithc]

@jamos-tay do look into how variables propagate via nested includes. Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

[yamgent]

Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

---

index.md

<includesrc="a.md"><spanid="someVariable">Original variable</span></include>

a.md

<includesrc="b.md"><spanid="someVariable">Nested include variable</span></include>

b.md

{{someVariable}}

---

[damithc]

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

Yes, that feels odd but from a reuse perspective that should be the behavior (opposite of how inheritance works). Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

[yamgent]

Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

That is indeed true, I think that is the desirable behaviour.

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"? Maybe "page variables" vs "global variables"? So we can say that "If a page uses a {{variable}}, then:

1. Page variables override global variables.
2. Outer page variables override inner page variables.

[damithc]

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"?

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

1. Page variables override global variables.

This should be the other way around too?

Maybe "page variables" vs "global variables"?

Or stay away from variables altogether. customization?

[yamgent]

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

Yes, we are using nunjucks behind the scene to implement variables.

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

This should be the other way around too?

Got it, global ones should have the highest priority. @jamos-tay you will have to rework this. :P

[damithc]

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

It would break the syntax introduced in this PR, right? :-) If the new syntax is introduced in this PR, it will not break existing features.
Anyway, it is a big change, and I'm not sure how hard it is to implement, especially if we cannot piggyback on nunjucks anymore. We can discuss on Saturday.

[jamos-tay]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

I'll need a way to detect whether a variable is a global or page variable though, but I think it can be done without using a hacky solution.

[damithc]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

Reusing the same syntax is appropriate only if a normal nunjucks variable behaves exactly the same way as the ones used for includes. Are we going to apply the same outermost-overrides behavior to normal nunjucks variables as well? i.e., those declared using {% set x = 'foo' %}

[jamos-tay]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

[damithc]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

Yup, this is a big decision. Better to make it now rather than later but we have to think it through. Although the issue came up in this PR, we are actually dealing with a much wider issue, namely, the need to support the outer-overrides-inner behavior that is central to reuse but opposite of the normal template behavior. The problem seem to be that we cannot support two opposing behaviors using the same internal library and/or syntax.

[jamos-tay]

After discussion:

We'll go with the original behavior:
Global variables override included variables
Outer include variables override inner include variables
{% set ... %} overrides all variables, but only applies to the page (excluding includes)

E.g.
variables.md: a = 1

a.md

<include src = b.md>
a = 2
b = 2
</include>

b.md

{{ a }} // 1
{{ b }} // 2
{% set a = 4 %}
{{ a }} // 4
<include src = c.md>
a = 3
b = 3
</include>

c.md

{{ a }} // 1
{{ b }} // 2

[jamos-tay]

Updated

Note: We currently do not support include variables referencing variables within the same include:

index.md:

<includesrc="a.md"><spanid="a">a</span><spanid="b">b & {{ a }}</span></include>

This is because {{ a }} gets rendered to an empty string when nunjucks processes index.md. Making this work would require some rework the page generation logic, which should be in another PR. This rework is also necessary to handle certain cases with set, so it can be fixed along with that.

[jamos-tay]

Rebased on master

[jamos-tay]

Updated

[yamgent]

Some more nits.

[jamos-tay]

Updated

[damithc]

Applied this in production nus-cs3281/2019@0d04ee4

Nice work @jamos-tay and reviewers @acjh@yamgent 👍

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes#545

What is the rationale for this request?

Users may want to create include file templates with variables that can be filled in, allowing reuse. However, they should only apply to each file. For example,

## {{ title }}
This is a level {{ level }} topic.

Allow users to specify variables in an include tag

What changes did you make? (Give an overview)

Allow users to specify Markbind variables in an include tag:

<include src=”foo.md”>
<span id="title"><span>New Title</span></span> // same syntax as variables.md
<span id="level">2</span>
</include>

These add on to (and overwrite) variables in _markbind/variables.md, and only apply to the context of the included file, and any files that the file includes.

We use nunjuck's or (default) syntax to allow for default values in case the variable is missing: {{ title or "Untitled" }}

Is there anything you'd like reviewers to focus on?

Users cannot reference other variables in the include, like this:

<include src=”foo.md”>
<span id="inner">Inner</span>
<span id="outer">Outer and {{ inner }}</span>
</include>

This is because {{ inner }} gets cleared when the original file is rendered. We can have a separate syntax for these, but it's a little unwieldly.

Referencing variables in _markbind/variables.md is okay.

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<spanid="color">red</span><fontcolor="{{ color }}">my text</font>

[jamos-tay]

@damithc Yes, tested and works

[damithc]

This PR affects the following statement, right?

[acjh]

Please use meaningful commit messages instead of "Pr fixes". 🙈

[jamos-tay]

Sorry =/

I'll do a quick rebase after it's approved

[yamgent]

1. Let's follow the same heading level as "<include> Inside an Included File".
2. Let's use "<include>" rather than "Include Tag" to follow the style of the rest of the documentation.

##### Specifying Variables in an `<include>`

[yamgent]

**It is possible to declare variables in an `<include>`.**

[yamgent]

... in an `<include>` tag:

[yamgent]

1. Fixing the grammar.
2. Rephrased the explanation of it being a template mechanism.

..., except that they only apply to the included file. They allow the included file to be reused as a template, for different source files using different variable values.

[damithc]

Side question: do they also work as nunjucks variables? e.g., {{ (size | int) - 1 }} would evaluate to 3 if <span id="size">4</span>

[jamos-tay]

Tested and it works - but do we want to document this? It seems like a small enough case, and it might confuse the user since it introduces another api

[damithc]

We already claim that MarkBind is generally compatible with nunjucks syntax. Here, we are using nunjucks syntax for a MarkBind feature. It's better to clarify that these variables are in fact equivalent to nunjucks variables declared within the scope of the included file. However, it can be mentioned in https://markbind.github.io/markbind/userGuide/markBindSyntax.html#support-for-nunjucks rather than under this feature. What do you think?

[damithc]

Does this work too? It's not related to includes but possibly related to how the feature is implemented.

<span id="color">red</span>
<font color="{{ color }}">my text</font>

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

[jamos-tay]

@jamos-tay make sure this is reflected somewhere in the docs too. i.e., together with the d-none attribute, this provides a convenient way to declare local variables and use them in a page.

@damithc Sorry I think I misunderstood, were you referring to

<span id="color">red</span>
<font color="{{ color }}">my text</font>

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes so I put the span in variables.md

I don't think there's a way to create a local variable for a page just yet, but it probably should be in its own PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

[jamos-tay]

Updated

[damithc]

within the same .md file? Because that doesn't work... I thought you were asking if you could put variables as element attributes.

I don't think there's a way to create a local variable for a page just yet, but it should be put it in a different PR. I'm pretty sure it requires separate logic (such as placing in the frontmatter) because of the way nunjucks is called.

I see. I thought we got that feature for free. Never mind then. It can be a feature to be added in the future.

[yamgent]

The wrong HTML tag is escaped, it didn't render correctly.

[yamgent]

Also it is not clear that the two different code exist in two different places, especially when you look at this portion in the live preview. Maybe indicate one is in variables.md and one is in index.md?

[yamgent]

Same problem (wrong HTML tag escaped).

[jamos-tay]

Updated, I decided to delete that section since there was a misunderstanding.

[yamgent]

1. The "Missing 'id'" part should actually be in front, in order to match the style of other error messages that we show as well.
2. I realise that the "Missing reference..." should be rephrased (because it is not a missing reference, but rather, a missing variable name), apologies about that.

So it should be something like this:

diff --git a/src/lib/markbind/src/parser.js b/src/lib/markbind/src/parser.js
index 69c40e5..b71b50f 100644
--- a/src/lib/markbind/src/parser.js+++ b/src/lib/markbind/src/parser.js@@ -215,8 +215,8 @@ Parser.prototype._preprocess = function (node, context, config) {
}
if (!child.attribs.id) {
// eslint-disable-next-line no-console
- console.warn(`Missing reference in ${element.attribs[ATTRIB_CWF]}\n`- + `Missing 'id' in variable for ${element.attribs.src} include.`);+ console.warn(`Missing 'id' in variable for referenced file: ${element.attribs.src}.\n`+ + `Missing variable name in ${element.attribs[ATTRIB_CWF]}\n`);
return;
}
includedVariables[child.attribs.id] = cheerio.html(child.children);

[damithc]

@jamos-tay do look into how variables propagate via nested includes. Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

[yamgent]

Ideally, the variable value should apply to nested includes too and outermost value should override values specified in inner includes.

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

---

index.md

<includesrc="a.md"><spanid="someVariable">Original variable</span></include>

a.md

<includesrc="b.md"><spanid="someVariable">Nested include variable</span></include>

b.md

{{someVariable}}

---

[damithc]

Do you mean that for the following scenario, the final output should be "Original variable"? The behaviour would look very odd to authors, since the inner page that is "closer" to the final included page should be allowed to have a say over the final variable value, rather than the outer page.

Yes, that feels odd but from a reuse perspective that should be the behavior (opposite of how inheritance works). Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

[yamgent]

Otherwise, someone who wants to resue this bit of code has no way to customize the code without modifying it directly, right?

That is indeed true, I think that is the desirable behaviour.

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"? Maybe "page variables" vs "global variables"? So we can say that "If a page uses a {{variable}}, then:

1. Page variables override global variables.
2. Outer page variables override inner page variables.

[damithc]

In order to make it easier to explain this behaviour, maybe we should consider adopting a different term rather than calling them all "variables"?

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

1. Page variables override global variables.

This should be the other way around too?

Maybe "page variables" vs "global variables"?

Or stay away from variables altogether. customization?

[yamgent]

That is a good idea. One issue is that we use the exact syntax used for nunjucks variables. Are we using nunjucks behind the scene to implement variables? This could become a problem because nunjucks variables are likely to behave differently from ours and users don't have a way to tell the two apart (note that a MarkBind file can contain some nunjucks code too). Is it possible to use a slightly different syntax e.g., {< foo >}?

Yes, we are using nunjucks behind the scene to implement variables.

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

This should be the other way around too?

Got it, global ones should have the highest priority. @jamos-tay you will have to rework this. :P

[damithc]

And yes there is a chance the authors may mix them up. We can explore using a different syntax in another PR (that would be a breaking change)?

It would break the syntax introduced in this PR, right? :-) If the new syntax is introduced in this PR, it will not break existing features.
Anyway, it is a big change, and I'm not sure how hard it is to implement, especially if we cannot piggyback on nunjucks anymore. We can discuss on Saturday.

[jamos-tay]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

I'll need a way to detect whether a variable is a global or page variable though, but I think it can be done without using a hacky solution.

[damithc]

Hmm... I think it might be possible to reuse the same syntax. I just have to check that if a variable already exists (i.e. declared by an outer file), don't overwrite it.

Reusing the same syntax is appropriate only if a normal nunjucks variable behaves exactly the same way as the ones used for includes. Are we going to apply the same outermost-overrides behavior to normal nunjucks variables as well? i.e., those declared using {% set x = 'foo' %}

[jamos-tay]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

[damithc]

Hmm... Nunjucks overwrites, so it'll use the inner most by default. We might be able to write an extension that will overwrite set's behavior. Alternatively we could strip out {% set %} tags and parse them manually.

If not, are we considering moving the entire variable system off nunjucks? i.e. {< variable >} everywhere? That might be a lot of work just for this feature... considering the global variables.md would have to be migrated over, we'd have to write our own parser logic, and also all older sites would break.

Yup, this is a big decision. Better to make it now rather than later but we have to think it through. Although the issue came up in this PR, we are actually dealing with a much wider issue, namely, the need to support the outer-overrides-inner behavior that is central to reuse but opposite of the normal template behavior. The problem seem to be that we cannot support two opposing behaviors using the same internal library and/or syntax.

[jamos-tay]

After discussion:

We'll go with the original behavior:
Global variables override included variables
Outer include variables override inner include variables
{% set ... %} overrides all variables, but only applies to the page (excluding includes)

E.g.
variables.md: a = 1

a.md

<include src = b.md>
a = 2
b = 2
</include>

b.md

{{ a }} // 1
{{ b }} // 2
{% set a = 4 %}
{{ a }} // 4
<include src = c.md>
a = 3
b = 3
</include>

c.md

{{ a }} // 1
{{ b }} // 2

[jamos-tay]

Updated

Note: We currently do not support include variables referencing variables within the same include:

index.md:

<includesrc="a.md"><spanid="a">a</span><spanid="b">b & {{ a }}</span></include>

This is because {{ a }} gets rendered to an empty string when nunjucks processes index.md. Making this work would require some rework the page generation logic, which should be in another PR. This rework is also necessary to handle certain cases with set, so it can be fixed along with that.

[jamos-tay]

Rebased on master

[jamos-tay]

Updated

[yamgent]

Some more nits.

[jamos-tay]

Updated

[damithc]

Applied this in production nus-cs3281/2019@0d04ee4

Nice work @jamos-tay and reviewers @acjh@yamgent 👍