Support for Template Toolkit 2 #1418

gflohr · 2018-05-04T09:12:34Z

This pull requests adds support for the Template Toolkit version 2.

The implementation assumes that the template processor is instantiated with default options. The options "INTERPOLATE", "START_TAG", "END_TAG", "TAG_STYLE", "OUTLINE_TAGS", and "ANYCASE" which signficantly change the default syntax, are not, or not fully supported.

Golmote

This looks great! Thanks for opening this new PR!

Could you take a look at my comments? There are quite a lot of them but it's mostly nitpicking.

Also, could you please run gulp and include the following generated files:

components.js
components/prism-tt2.min.js
plugins/autoloader/prism-autoloader.js
plugins/autoloader/prism-autoloader.min.js
plugins/show-language/prism-show-language.js
plugins/show-language/prism-show-language.min.js

(This should be all files modified by the gulp command, if I did not forget any...)

Golmote · 2018-05-04T18:09:11Z

components/prism-tt2.js

+		// A colon is not allowed inside variables but we want to catch things
+		// like [% USE Project::Specials %]
+		variable: {
+			pattern: /[a-z][:_a-z0-9]*(?:[\011-\015\040]*\.[\011-\015\040]*(?:\d+|\$?[a-z][:_a-z0-9]*))*/i,


Couldn't [\011-\015\040] be replaced with \s? Both don't have exactly the same meaning but it would make the regexp more clear I think.
Also, you can replace _a-z0-9 with \w.
Do you need the colon : inside the second part too?

Okay, with syntactically correct code \s is equivalent. Same for\w. I will change it to your preferences.

The colon was for fully qualified plug-in names (Foo::Bar). I thought they are supported but it looks like they actually don't work. So I will remove the colons. Even if they did work, the colons will then be highlighted as punctuation which is also okay.

Golmote · 2018-05-04T18:12:25Z

components/prism-tt2.js

+
+	Prism.languages.tt2 = Prism.languages.extend('clike', {
+		comment: {
+			pattern: /#.*|\[%#[^]*?%\]/,


We've decided to use [\s\S] instead of [^] in #1107. Could you change this?

Sure, I didn't know that.

Golmote · 2018-05-04T18:13:41Z

components/prism-tt2.js

+			pattern: /#.*|\[%#[^]*?%\]/,
+			lookbehind: true
+		},
+		keyword: /\b(?:GET|CALL|SET|DEFAULT|INSERT|INCLUDE|PROCESS|WRAPPER|BLOCK|IF|UNLESS|ELSIF|ELSE|SWITCH|CASE|FOREACH|IN|WHILE|FILTER|USE|MACRO|RAWPERL|PERL|TRY|THROW|CATCH|FINAL|NEXT|LAST|RETURN|STOP|CLEAR|META|TAGS|DEBUG|END)\b/,


Considering the large number of keywords here, you might want to consider ordering them alphabetically, for easier addition/removal in the future.

They are in the order of the directives documentation. Switch to alphabetical or go with the docs?

Ok, could you then add a link to this doc page, in a comment above the regexp?

Golmote · 2018-05-04T18:18:37Z

components/prism-tt2.js

+
+	var operatorRE = /(?:=>|==|!=|<=|<|>=|>|=|&&|\|\||\||!|and|or|not)/;
+
+	Prism.languages.insertBefore('tt2', 'number', {


I think you should add a line like delete Prism.languages.tt2['operator']; before you call insertBefore, so that the operator property can effectively be inserted. If it already exists, it will have unexpected behaviour.

Okay, I didn't know that and see whether that also fixes the FIXME below.

Golmote · 2018-05-04T18:19:19Z

components/prism-tt2.js

+		}
+	});
+
+	// FIXME! Is this repetition really necessary?


See my previous comment regarding the deletion of the operator property. It should allow you to remove those lines.

Golmote · 2018-05-04T18:56:39Z

tests/languages/tt2/keyword_feature.test

+[% META title = 'Hello!' %]
+[% TAGS [@ @] %]
+[% DEBUG on %]
+[% DEBUG off %]


If you were to reorder the keywords in the regexp, it would be nice to reorder the tests alphabetically too. Note that tests do not need to be valid structures in TT2.
It's perfectly ok if you test something like:

... [% ELSE %] [% ELSIF %] [% END %] ...

I understood that but I found a bug or two because of the more realistic test cases. Is that really a problem? It's better test coverage in the end.

Reordering: see above, whatever you prefer. Although ELSE before IF looks really strange to me. ;)

No, it's not really a problem. It was just a suggestion, feel free to let it as it is.

Golmote · 2018-05-04T18:59:18Z

tests/languages/tt2/keyword_feature.test

+
+[
+	["tt2", [["delimiter", "[%"], ["keyword", "GET"], 
+			 ["variable", "foo"], ["delimiter", "%]"]]],


Identation is a bit weird in this file. Maybe you can keep it concise (I guess it was the intention here) doing something like:

[ ["tt2", [ ["delimiter", "[%"], ["keyword", "GET"], ["variable", "foo"], ["delimiter", "%]"] ]],

The problem is the tabwidth. I had assumed 4 here. I will reformat with 8 (also for your other remarks on that issue).

Golmote · 2018-05-04T19:00:00Z

tests/languages/tt2/operator_feature.test

+[
+	["tt2", [["delimiter", "[%"],
+			 ["variable", "fat"], ["operator", "=>"], ["variable", "comma"],
+			 ["delimiter", "%]"]]],


Same comment here regarding indentation.

Golmote · 2018-05-04T19:02:24Z

tests/languages/tt2/string-interpolation_feature.test

+		 	["double-quoted-string",
+			 	["\"Hello, ",
+				 ["variable", "$name"],
+				 "!\""]


Indentation seems a bit off here too.
Something like the following would be easier to read:

["double-quoted-string", [ "\"Hello, ", ["variable", "$name"], "!\"" ],

Golmote · 2018-05-04T19:08:36Z

tests/languages/tt2/variable_feature.test

+	world
+	hands
+	knots
+%]


Could you add a test for the Project::Specials kind of variables here? I know it's technically already tested in the test for keywords, but since it's handled by the variable pattern, it definitely belongs here too.

Looks like it's not supported anyway, see above. So I'll remove the colons from the variable regex as well.

Also use \w.

See PrismJS#1107.

gflohr · 2018-05-05T09:53:08Z

I think I am through now except for the generated files which I will push as soon as I have your okay.

I have ordered the keywords alphabetically, easier to maintain.

And I have removed the Namespace::Plugin feature as it seems not to work in templates.

Golmote

Thanks! I think you missed some of my comments, maybe because of the "Hidden conversation" ellipsis Github adds. I reposted them, so that they appear again below. Can you take a look?

Golmote · 2018-05-09T07:07:12Z

components/prism-tt2.js

+	delete Prism.languages.tt2['operator'];
+	delete Prism.languages.tt2['variable'];
+	Prism.languages.insertBefore('tt2', 'number', {
+		operator: /(?:=>|==|!=|<=|<|>=|>|=|&&|\|\||\||!|and|or|not)/,


You could probably combine some patterns here to avoid backtracking.
Also, the parentheses around the whole regexp are not needed.
Finally, wouldn't it be safer to add \b assertions around the keyword operators?

Something like /=[>=]?|!=?|<=?|>=?|&&|\|\|?|\b(?:and|or|not)\b/?

True, that's more efficient and the \b also makes the greedy variables unnecessary, see below.

But - sorry for being a smart ass ;) - matching a 58 million character string without any space against \s takes about 50 % longer than matching against the more correct /[\011-\015\040]/, both in Firefox and Chrome.

Hahaha. I take good note of this. And if you really want to revert this change, you might. But you'd have to add a comment above the regexp, specifying what are those characters, something like
// [\011-\015\040] -> Tab, LF, vertical tab, form feed, CR and space
or use the character escapes /[\t\n\v\f\r ]/. Just so that the meaning is obvious without looking at an ASCII reference table. 😉

Golmote · 2018-05-09T07:07:40Z

examples/prism-tt2.html

+%]
+[% # this is a comment
+   theta = 20      # so is this
+   rho   = 30      # <aol>me too!</aol>


You'll probably need to escape the < characters using < in <aol> and </aol>.

Golmote · 2018-05-09T07:07:54Z

examples/prism-tt2.html

+  as variables here.</li>
+  <li>The
+  <a href="http://template-toolkit.org/docs/manual/Config.html#section_ANYCASE">ANYCASE</a>
+  option is not supprted.</li>


There is a typo here: "supported".

Golmote · 2018-05-09T07:08:01Z

tests/languages/tt2/comment_feature.test

+----------------------------------------------------
+
+[
+    ["tt2",


Please indent this line with a tab.

Golmote · 2018-05-09T07:08:53Z

components/prism-tt2.js

+		operator: /(?:=>|==|!=|<=|<|>=|>|=|&&|\|\||\||!|and|or|not)/,
+		variable: {
+			pattern: /[a-z]\w*(?:\s*\.\s*(?:\d+|\$?[a-z]\w*))*/i,
+			greedy: true


Do variables really need to be greedy? The greedy behaviour has a bit of a performance hit, so you should make sure you actually require it.

With the \b around the keyword-like operators and|or|not it is no longer needed. Thanks for the hint.

Golmote · 2018-05-09T07:09:14Z

components/prism-tt2.js

+			alias: 'string',
+			inside: {
+				variable: {
+					pattern: /\$(?:[a-z][_a-z0-9]*(?:\.(?:\d+|\$?[a-z][_a-z0-9]*))*)/i,


You can replace [_a-z0-9] with \w.

Golmote · 2018-05-09T07:11:13Z

components/prism-tt2.js

+			inside: {
+				variable: {
+					pattern: /\$(?:[a-z][_a-z0-9]*(?:\.(?:\d+|\$?[a-z][_a-z0-9]*))*)/i,
+					greedy: true


Same comment here about the greedy feature.

Also use \w instead of explicit character class.

gflohr · 2018-05-09T09:42:26Z

I have applied all your changes. Push the generated stuff now?

Golmote · 2018-05-09T18:44:08Z

Yes please!

gflohr · 2018-05-10T05:04:37Z

Pushed.

And thanks for the review process. That was very instructive for me.

Golmote · 2018-05-10T09:10:27Z

Thanks for contributing!

* gh-pages: (33 commits) Add double-class specificity hack (#1435) Moved tutorial link to the end of the list Make line-numbers styles more specific Fix mixed content warning Create CNAME Delete CNAME Update documentation for node & webpack usage Handle optional dependencies in `loadLanguages()` (#1417) Add `objectpascal` as an alias to `pascal` (see #1426) Add support for XQuery. Fix #1405 (#1411) Website: Fix Download page not handling multiple dependencies when from Redownload URL JSX: Add support for fragments short syntax. Fix #1421 Support for Template Toolkit 2 (#1418) ASP.NET should require C# Run gulp Move guard into conditional and check for language Don't process language if block language not set JSX: Allow for two levels of nesting inside JSX tags. Fix #1408 Add missing reference to issue in specific test. Powershell: Allow for one level of nesting in expressions inside strings. Fix #1407 ... # Conflicts: # components/prism-kotlin.js

gflohr added 3 commits May 4, 2018 10:57

support for Template Toolkit 2

33e23a0

optimized quoted string regexes

50cff77

optimized double-quoted string regex

7baa4d3

Golmote requested changes May 4, 2018

View reviewed changes

gflohr changed the title ~~Supprt for Template Toolkit 2~~ Support for Template Toolkit 2 May 4, 2018

gflohr added 7 commits May 5, 2018 11:48

fully qualified tt2 plug-in names don't work

cf49406

Also use \w.

do not use empty character class

72e1320

See PrismJS#1107.

put keyword tests in alphabetical order

51599d9

ordered tt2 keywords alphabetically

d8d790a

removed redundant operator rule for tt2

86995c2

removed possibly existing old rules before inserti

60b7a0e

indentation

88e5662

gflohr added 2 commits May 5, 2018 13:05

allow backslash in front of lf in tt2 strings

9339e8b

indentation

a0b80aa

Golmote requested changes May 9, 2018

View reviewed changes

gflohr added 4 commits May 9, 2018 12:12

avoid backtracking

41f8c2f

escape xml special characters

34f4ec8

indent with tab instead of spaces

a30d0d6

greedy is no longer needed for variables

1479a25

Also use \w instead of explicit character class.

Golmote approved these changes May 9, 2018

View reviewed changes

re-generated with gulp

bbd01bd

Golmote merged commit e063992 into PrismJS:gh-pages May 10, 2018


		var operatorRE = /(?:=>\|==\|!=\|<=\|<\|>=\|>\|=\|&&\|\\|\\|\|\\|\|!\|and\|or\|not)/;

		Prism.languages.insertBefore('tt2', 'number', {

Support for Template Toolkit 2 #1418

Support for Template Toolkit 2 #1418

Conversation

gflohr commented May 4, 2018

Golmote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Golmote May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gflohr commented May 5, 2018

Golmote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gflohr commented May 9, 2018

Golmote commented May 9, 2018

gflohr commented May 10, 2018

Golmote commented May 10, 2018

Golmote May 4, 2018 •

edited

Loading