(handlebars) support for segment-literals and escaped mustaches #2184

nknapp · 2019-10-12T19:09:08Z

This PR adds support for rather exotic expressions like

{{[property name containing spaces]}}
{{"property name containing spaces and [] square brackets"}}
{{'property name containing spaces and "" quotes'}}

{{helper [property name containing spaces]}}
{{property.[property name containing spaces]}}

and most importantly

{{"property containing a closing }} mustache"}} followed by xml {{and another mustache}}

I have set the relevance values such that no changes in auto-detection heuristics takes place.

Update: Added support for escaped mustaches as well:

\{{no-expression}} {{expression}}

\{{{no-expression}}} {{expression}}

\{{#no}} block \{{/no}} {{expression}}

\{{\{{no}}}} block \{{\{{/no}}}} {{expression}}

Update 2: looks like GitHub's syntax highlighter isn't supporting that.

nknapp · 2019-10-12T19:09:42Z

The PR is based on the commits of #2182 so we should probably wait until that one is merged.

joshgoebel · 2019-10-12T19:15:57Z

Lets not use functions if we don't need them. I think almost most of those cases can be normal variables. Only prefer the function when you need a unique object to make the parser happy.

For example no reason for EXPRESSION to be a function.

joshgoebel · 2019-10-12T19:21:54Z

src/languages/handlebars.js

-    return  {
-      className: 'name',
-      begin: /[a-zA-Z\.-]+/,
+function handlebars(hljs) {


Lets not change the intro, some of the existing build process is very picky about this... so just function(hljs) please.

What I like about functions, is that you can write them top-down in the order in which you would read the code.

function main() { if(isThis()) { doThat() } } function isThis() {...} function doThat() {...}

Mostly, functions are used the other way around,

function isThis() {...} function doThat() {...} function main() { if(isThis()) { doThat() } }

but I think it makes the code less readable, because when you start reading at the top, you get all the helper functions first and come to the actual calling code last.

But in that case, I actually started replace some functions by variables, and had all tests failing. I can try some more if you like.

nknapp · 2019-10-12T20:22:05Z

To be honest, I'm not happy with changing this, because it really messes up the reading order.
I actually would move all functions to the bottom (behind the return) and even split more out of them.

Maybe I'll try what it does to the minified code.

Update: Yes, I believe it will increase the code size to have everything in a function.

joshgoebel · 2019-10-12T20:26:24Z

To be honest, I'm not happy with changing this, because it really messes up the reading order.

To me it's a little bit of a consistency thing (with other grammars) and the functions do add a lot of boilerplate - but I'm not hard set myself, I could be easily persuaded. @egor-rogov Any strong opinions on this?

I do think it's perhaps a little overkill if you LITERALLY have just a variable and that then you should just deal with the order not being perfect. :-) This is why we need ES6 though then the new hoisting rules could help us without all this drama about functions. ;-) Plus then the => syntax makes it matter a whole lot less anyways. :-)

Are there any implications here about minification and what's possible or not (whether they are variables vs functions)? Ha, is that what you meant? Yeah, that'd be useful to know.

nknapp · 2019-10-12T21:15:52Z

I just started writing a really long, but it is too long. Maybe I'll write a blog post about this.
The short version is: I try to apply principles from the book "Clean Code" by Robert C. Martin. But in this particular case it does not work because "Clean Code" is mostly about Java and byte size of the compiled program is not critical there.

So I agree with your request.

I would extract even more functions, but I what I have tried would result in 300 bytes more code.

In fact, if we find out why reusing the "starts" object is a problem, and fix it, then we should remove the other functions as well.

If you are interested: The minified code shows why that is is the case

hljs.registerLanguage("handlebars", function (e) {
  var t = {"builtin-name": "each in with if else unless bindattr action collection debugger log outlet template unbound view lookup"},
      a = {b: /("[^"]+"|'[^']+'|\[[^\]]+\]|\w+)/};

  function n() {
    return e.inherit(r(), {cN: "name"})
  }
...

The function, (), {, return, } of function n()... cannot be removed, whereas a={...} just requires a , before the a and a =. Extracting a variable is always smaller than extracting a function (in the resulting minified program)

joshgoebel · 2019-10-12T21:21:06Z

I agree in general. I love Clean Code and clean code. Guess right now I think of the grammars more as data than code, though.

I also think we've been too paranoid in the past about size and gzip solves most of these problems. So I think mostly I was coming from a style and consistency standpoint when I made the original request to stick with variables. We'll see what Egor says. :-)

joshgoebel · 2019-10-13T12:53:51Z

src/languages/handlebars.js

+  var BUILT_INS = {'builtin-name': 'each in with if else unless bindattr action collection debugger log outlet template unbound view lookup'};
+
+  var IDENTIFIER_PLAIN_OR_QUOTED = {
+    begin: /("[^"]+"|'[^']+'|\[[^\]]+\]|\w+)/


Wouldn't back references be a better use here rather than repeating yourself 3-4 times?

If at all, only the first two alternatives could be combined using back-referencens.

Non-greedy matching would make it better readable (and it must be .*, not .+, because '', "" and [] are also allowed (in theory):

/".*?"|'.*?'|\[.*?\]|\w+/

But I don't think that this is intuitive:

/(['"]).*?\1|\[.*?\]|\w+/

I actually wanted to use:

var IDENTIFIER_PLAIN_OR_QUOTED = { begin: /\w+/, variants: [ {begin: /"/, end: /"/}, {begin: /'/, end: /'/}, {begin: /\[/, end: /]/} ] };

but that didn't work. The main begin-pattern /\w+/ seemed to be ignored in this case.

Non-greedy is more readable.

Variants are alternatives to the main block, not "in addition to" or "after"... If you just removed begin would the variants not work?

joshgoebel · 2019-10-13T20:37:33Z

test/markup/handlebars/escaped-mustaches.txt

+
+\{{#no}} block \{{/no}} {{expression}}
+
+\{{\{{no}}}} block \{{\{{/no}}}} {{expression}}


What if I want to skip a comment like snippet? Won't those rules still match?

No. You are right, that one is still missing. I'll add that.

A \ in front of the mustache is escaped by another \ in front of it.
This means that the expressions in

\\{{expression}} \\\{{expression}} \\\\{{expression}}

are all evaluated. I have added a commit that skips the escaping if another backslash appears in front of the backslash.

I mean don’t the escape rules need to come before the comment rules to allow escaping a comment?

I have added a test for escaped comments and it passed without modification of the "handlebars.js" file.

I think, since the match of \{{ has a lower index than the matching {{, it will always be evaluated first.

Also, after writing my own review, I felt I had to extract the rules into there own variables with descriptive names. Sorry for all the changes that you didn't ask about.

I also added consistent escaping of { and } in the regular expressions of the language. I don't think that is really necessary and I thought about removing all backslashes at first. But since the regex are assembled into larger once, we cannot be sure. There ARE contexts where a { needs to be escaped.

I think, since the match of {{ has a lower index than the matching {{, it will always be evaluated first.

Yeah, I thought I saw the comment regex coming FIRST, which is why i was curious.

Also, after writing my own review, I felt I had to extract the rules into there own variables with descriptive names.

Do the names have meaning if you know Handlebars really well? I'm actually not sure it's a win (while normally I'd call it a win)... but to me I SEE the patterns and that means something to me, but I see the labels you've assigned them and that means nothing to me.

If it were me I'd add comment examples beside them.

STANDARD_BLOCK, // {{ ... }}

joshgoebel · 2019-10-14T21:16:27Z

Sorry I might have just messed ya up :) You can just force push without my commit if it's no good.

joshgoebel

You all good now? This is looking pretty sharp.

nknapp · 2019-10-15T14:24:18Z

I'm think its good like that

nknapp · 2019-10-15T14:25:49Z

Thanks for all the review work you put in there.

nknapp · 2019-10-15T14:28:35Z

Argh no. I noticed that I removed the yield keyword. It should be added again. Its not important for Handlebars but it was in there and if keyword cleanup is something we want to do, of should be a separate PR

- square brackets and quotes are used in Handlebars to escape identifiers, should they contain invalid characters. This commit makes sure that "abc}}abc" does not close the mustache- expression. - keep values of "relevance" such that autodetection still keeps the same relevance values.

- called IDENTIFIER_PLAIN_OR_QUOTED now, because that is what it is.

nknapp · 2019-10-15T14:43:42Z

I have reverted the keyword list and rebased on master. If the tests pass, from my point of view, you can merge.

joshgoebel · 2019-10-15T14:46:26Z

src/languages/handlebars.js

@@ -6,63 +6,91 @@ Description: Matcher for Handlebars as well as EmberJS additions.
 Website: https://handlebarsjs.com
 Category: template
 */
-function(hljs) {
+function (hljs) {


This kind of change just bit me today. :) Although a few of the other languages are also so different.

joshgoebel · 2019-10-15T14:46:42Z

@nknapp Thanks for all the hard work!

joshgoebel requested changes Oct 12, 2019

View reviewed changes

joshgoebel added enhancement An enhancement or new feature language labels Oct 12, 2019

joshgoebel requested changes Oct 13, 2019

View reviewed changes

joshgoebel changed the title ~~Expression variants~~ (handlebars) Expression variants Oct 13, 2019

joshgoebel added this to the 9.15.12 milestone Oct 13, 2019

nknapp changed the title ~~(handlebars) Expression variants~~ (handlebars) support for segment-literals and escaped mustaches Oct 13, 2019

joshgoebel requested changes Oct 13, 2019

View reviewed changes

nknapp force-pushed the expression-variants branch from 278cedc to e7a68dd Compare October 14, 2019 21:28

joshgoebel approved these changes Oct 15, 2019

View reviewed changes

nknapp force-pushed the expression-variants branch from 640db25 to 3c7adae Compare October 15, 2019 14:39

nknapp added 9 commits October 15, 2019 16:40

fix(handlebars): remove intro

0eea32e

fix(handlebars): change EXPRESSION to be a variable

dc288bd

- called IDENTIFIER_PLAIN_OR_QUOTED now, because that is what it is.

fix(handlebars): add some missing semicolons

5acf050

fixup: make IDENTIFIER_PLAIN_OR_QUOTED pattern more readable

a2c5585

fix(handlebars): support for escaped mustaches

2eb615e

fix(handlebars): support for escaped backslashes in front of a mustache

e50a4ca

fix(handlebars): add test for escaping comments

c477523

fix(handlebars): extract variables for more understandability

264e0cd

nknapp added 3 commits October 15, 2019 16:40

fix(handlebars): escape curly braces in all regex

23dcb37

fix(handlebars): re-inline rules

671e3c9

fix(handlebars): re-add "yield" keyword

75327a1

nknapp force-pushed the expression-variants branch from 3c7adae to 75327a1 Compare October 15, 2019 14:42

joshgoebel reviewed Oct 15, 2019

View reviewed changes

joshgoebel merged commit bdf87d2 into highlightjs:master Oct 15, 2019

nknapp mentioned this pull request Oct 15, 2019

update changelog #2208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(handlebars) support for segment-literals and escaped mustaches #2184

(handlebars) support for segment-literals and escaped mustaches #2184

nknapp commented Oct 12, 2019 •

edited

nknapp commented Oct 12, 2019

joshgoebel commented Oct 12, 2019

joshgoebel Oct 12, 2019

nknapp Oct 12, 2019

nknapp commented Oct 12, 2019 •

edited

joshgoebel commented Oct 12, 2019 •

edited

nknapp commented Oct 12, 2019 •

edited

joshgoebel commented Oct 12, 2019

joshgoebel Oct 13, 2019

nknapp Oct 13, 2019 •

edited

joshgoebel Oct 13, 2019

joshgoebel Oct 13, 2019

nknapp Oct 14, 2019

nknapp Oct 14, 2019

joshgoebel Oct 14, 2019

nknapp Oct 14, 2019

joshgoebel Oct 14, 2019

joshgoebel Oct 14, 2019

joshgoebel Oct 14, 2019

joshgoebel commented Oct 14, 2019

joshgoebel left a comment

nknapp commented Oct 15, 2019

nknapp commented Oct 15, 2019

nknapp commented Oct 15, 2019

nknapp commented Oct 15, 2019

joshgoebel Oct 15, 2019

joshgoebel commented Oct 15, 2019


		\{{#no}} block \{{/no}} {{expression}}

		\{{\{{no}}}} block \{{\{{/no}}}} {{expression}}

(handlebars) support for segment-literals and escaped mustaches #2184

(handlebars) support for segment-literals and escaped mustaches #2184

Conversation

nknapp commented Oct 12, 2019 • edited

nknapp commented Oct 12, 2019

joshgoebel commented Oct 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nknapp commented Oct 12, 2019 • edited

joshgoebel commented Oct 12, 2019 • edited

nknapp commented Oct 12, 2019 • edited

joshgoebel commented Oct 12, 2019

Choose a reason for hiding this comment

nknapp Oct 13, 2019 • edited

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

joshgoebel commented Oct 14, 2019

joshgoebel left a comment

Choose a reason for hiding this comment

nknapp commented Oct 15, 2019

nknapp commented Oct 15, 2019

nknapp commented Oct 15, 2019

nknapp commented Oct 15, 2019

Choose a reason for hiding this comment

joshgoebel commented Oct 15, 2019

nknapp commented Oct 12, 2019 •

edited

nknapp commented Oct 12, 2019 •

edited

joshgoebel commented Oct 12, 2019 •

edited

nknapp commented Oct 12, 2019 •

edited

nknapp Oct 13, 2019 •

edited