Allow # to be used as an inline comment tag

[dylanahsmith]

Fixes #1393
Depends on Shopify/liquid-c#144

@Shopify/guardians-of-the-liquid

Allow # to be used as an inline comment tag, ignoring everything after it in the tag.

For example, instead of

{% comment %}Single line comment{% endcomment %}

you can use

{% # Single line comment %}

Spaces around # aren't necessary, so this also works

{%#This also works %}

Using this after another tag (e.g. {% assign discouraged = true # This comment is parsed as part of the assign tag %}) would cause it to be parsed as part of that tag, so is not recommended since we can't ensure consistency between tags and backwards compatibility (especially considering that there are application defined tags). Instead, use a separate tag

{% assign recommended = true  %}{%# Comment on this tag -%}

Whitespace control characters work the same way as other tags.

This inline comment also works in liquid tags, so instead of

{% liquid
  echo "..."
  comment Single line comment
  endcomment
  echo "..."
%}

you can use

{% liquid
  echo "..."
  # Works well for single line comments
  # or even multi-line comments
  echo "..."
%}

[pushrax]

{% assign discouraged = true # This comment is parsed as part of the assign tag %}

I can definitely picture people learning about comment syntax by example and then assuming this would work. We need some kind of strategy there.

[dylanahsmith]

Perhaps we should assess whether # can be supported in all standard and Shopify tags to determine if we can support this consistently or whether it would result in any ambiguities. I think it is likely that we can add support for it. If so, I think developers would appreciate us adding support for that.

From an implementation perspective, we wouldn't be able to just ignore everything after the first # so we don't break syntax that supports # (e.g. a quoted string), but we could add a method like Liquid::Parser#consume_markup_end that can raise a parse error if it doesn't find an inline comment or end of string.

[tjoyal]

Same feedback:

  def test_conditional
    assert_template_result('true', '{% if true # comment %}true{% else %}false{% endif %}')
    # InlineCommentTest#test_conditional:
    # Liquid::SyntaxError: Liquid syntax error (line 1): Unexpected character # in "true # comment"
  end

  def test_assign
    assert_template_result('true', '{% assign test = true # comment %}{{test}}')
    # InlineCommentTest#test_assign:
    # Liquid::SyntaxError: Liquid syntax error (line 1): Unexpected character # in "{{true # comment }}"
  end

Not being familiar with the parsing process, is the only option to implement comments as a tag? Would we want to have a way to strip these completely when generating an intermediary representation so they are not part of the final execution path? I would rather not have tags even be aware there was comments in the first place.

[dylanahsmith]

Not being familiar with the parsing process, is the only option to implement comments as a tag?

Comments are already implemented as tags, doing the same for inline comments makes the changes simpler and less likely to introduce a breaking change.

Basically, I'm avoiding introducing a special case where there isn't a 1-1 correspondence between the tag syntax and a tag node. Breaking that would make the implementation more awkward, thus supporting inline comments at the end of other tags wouldn't introduce an InlineComment tag node for the same reason.

Would we want to have a way to strip these completely when generating an intermediary representation so they are not part of the final execution path?

We can remove these when compiling to liquid-c VM code as will be done for the Comment tag in Shopify/liquid-c#96

I would rather not have tags even be aware there was comments in the first place.

This is just an implementation detail that should be opaque to all the other tags, since tag classes shouldn't be coupled to each other

[tjoyal]

If # is a tag and we make it clear, even if somewhat confusing at first, it does map to the behaviour me and @pushrax are describing. Works as expected and move forward with the proposition?

[dylanahsmith]

Do you think we should ship this as is then iterate by trying to follow-up with support for comments at the end of tags? Or should I hold off on that exploration?

[tjoyal]

If we consider it a tag, then maybe it's even better(easier to form a mental model) that you cannot add it at the end of another tag.

The spaces around # aren't necessary, so this also works

Reviewing back, should this not be made possible then?

[pushrax]

Spaces around tags aren't strictly required in any case, e.g. {%endfor%} is valid. The internal syntax of some tags requires spaces though, e.g. {%for foo in bar%}.

I don't really have much of an opinion on comments at the end of tags, as long as it crashes gracefully.

[tjoyal]

My feedback was not for space before, but after a tag.

{% assign_some_extra_chars test = 1 %} is obviously invalid
My take is that {% #test %} should also be invalid and require a space (the parser behave differently the way the regex is written).

[dylanahsmith]

My take is that {% #test %} should also be invalid and require a space (the parser behave differently the way the regex is written).

I guess it makes sense to have that be invalid until we add support for comments at the end of tags.

[dylanahsmith]

I've updated the code to require a space after # and updated the PR description accordingly.

[tjoyal]

What's is the plan here? Will you merge it remove current change so it point to master?

[dylanahsmith]

The liquid-c change can be safely merged first, then I'll update this to point back to master before merging.

[pushrax]

{% assign_some_extra_chars test = 1 %} is obviously invalid

That is because the underscores are part of the tag name and there is no tag called assign_some_extra_chars. {% foo#bar %} or {% foo!bar %} isn't invalid if the implementation of foo supports that syntax.

class Foo  < Liquid::Tag
  def initialize(tag_name, arg, tokens)
    @arg = arg
  end
  def render(_)
    @arg.to_s
  end
end

Liquid::Template.register_tag('foo', Foo)

puts Liquid::Template.parse("{%foo#bar%}").render # => "#bar"
puts Liquid::Template.parse("{%foo!bar%}").render # => "!bar"

I think most people would expect the comment syntax to work without the space.

[dylanahsmith]

Oh right, spaces after the tag name are just needed to separate it from other word characters, since it just requires a non-word character to end the tag.

I've updated the code to require a space after # and updated the PR description accordingly.

I've removed that last commit

[tjoyal]

I think we are good now! I'd love to see @pushrax case in the tests as if anything is to break in future iterations I think it is going to be that (since tag registration are global, I'm not sure it can be done without some tinkering).

[samdoiron]

Worth noting that this removes a lot of the benefit of treating # as its own tag, because it means the concern leaks into the syntax of other tags too.

This also has implications for the API, since we'd have to either

1. Do pre-processing on all tag input before passing it, which is a breaking change in a strict sense
2. Only support it for our own tags, which could lead to inconsistencies for other clients.

The second of these may still be the right call, but special-casing those tags could be more complex in Liquid-C.

[tobi]

What exactly is the benefit of treating it as a tag? My (limited!) experience with parser creation suggests that comments are usually dropped very early in the process of parsing/lexing and this feels like the right approach for liquid as well.

[timdmackey]

Using this after another tag (e.g. {% assign discouraged = true # This comment is parsed as part of the assign tag %}) would cause it to be parsed as part of that tag, so is not recommended since we can't ensure consistency between tags and backwards compatibility (especially considering that there are application defined tags).

Do you think we should ship this as is then iterate by trying to follow-up with support for comments at the end of tags? Or should I hold off on that exploration?

I guess it makes sense to have that be invalid until we add support for comments at the end of tags.

I've been thinking about how to extend inline comments to all tags, and it occurred to me—if the plan is to implement # as a new tag to avoid breaking changes, what if # comments were *also* implemented as a filter? If the filter ignores any "arguments" (text following the # key), and if the : isn't strictly necessary, then you could do stuff like this:

{%
  assign myvar = "hello world"
  | # add exclamation marks
  | append: "!!!!"
  | # convert to JSON format
  | json
%} {{ myvar }}

{% assign myvar = "hello world" | # add question marks: | append: "????" | # convert to JSON format: | json %} {{ myvar }}

{{
  product
  | # get image thumbnail:
  | img_url: "240x240"
}}

{{ product | # get fullsize image: | img_url: "1600x1600" }}

Interestingly, all 4 of these examples already render without issue in Shopify's theme editor due to it's lax error handling. They only raise a "potential issue" warning inside of a development shop:

[dylanahsmith]

Not all tags support filters, so adding a | character before a tag doesn't help at all.

Interestingly, all 4 of these examples already render without issue in Shopify's theme editor due to its lax error handling.

Yeah, the lax parsing makes it hard to extend the syntax in a backwards compatible way, since avoiding breaking working liquid code (without usage testing) requires finding something that is currently considered an error. Essentially, syntax errors reserve syntax for future features. Instead, the lax parser just scans over a regular expression that it doesn't match on to find the filter, including the # character, so we risk breaking liquid code by changing that behaviour from skipping just the # character to skipping everything after it on the line.

[timdmackey]

we risk breaking liquid code by changing that behaviour from skipping just the # character to skipping everything after it on the line.

Not sure this is helpful in the overall picture, but currently doesn't the parser just skip everything until the next pipe character? It appears to me that the parser is failing to match the first word (#), and so it jumps ahead to the next filter. Here's the output of my above code:

 "hello world!!!!"

 "hello world????"

//cdn.shopify.com/shopifycloud/shopify/assets/no-image-2048-5e88c1b20e087fb7bbe9a3771824e743c244f437e4f8ba93bbf7b11b53f7824c_240x240.gif

//cdn.shopify.com/shopifycloud/shopify/assets/no-image-2048-5e88c1b20e087fb7bbe9a3771824e743c244f437e4f8ba93bbf7b11b53f7824c_1600x1600.gif

[dylanahsmith]

currently doesn't the parser just skip everything until the next pipe character?

It does that for the first pipe character. After that, it treats anything that doesn't match Liquid::Variable::FilterParser.

Not sure this is helpful in the overall picture

What would help would be if less invalid code was saved in Shopify, so it doesn't make it look like code that depends on these quirks. This invalid liquid can be tested using this gem directly (e.g. ruby -rliquid -e 'print Liquid::Template.parse(STDIN.read).render. < test.liquid).

[dylanahsmith]

If I understand correctly, this test is saying that a space is required after #.

[dylanahsmith]

I don't understand the reason you are proposing supporting this style. Does it have any semantic difference?

Other than this test case, it seems like # is supposed to treat the rest of the line as a comment, but here it seems to be integrated into the start tag. So does this have different semantic for multi-line comments like the following?

{%#- This is clearly a comment
  echo "but how about this line?" -#%}

I'm also unsure if you are proposing that the -#%} end tag should have any significance over -%}.

[colinbendell]

Worth noting that NunJucks (a popular templating language in SSG that is a near relative to liquid) uses the {# bla #} convention. While NunJucks has had a different evolutionary path than liquid, it might be best, for developer ergonomics, to adopt the existing pattern instead of introducing a new pattern.

[sidebar: webstorm integration is really convenient here. comment and uncomment are really easy]

[ADTC]

Interestingly, all 4 of these examples already render without issue in Shopify's theme editor due to it's lax error handling. They only raise a "potential issue" warning inside of a development shop:

@timdmackey but does it actually render anything when you load the page? Sometimes, Liquid errors appear dumped in the rendered page even if the theme code editor has no complaints. I'd love to see your comment updated with a screenshot of that 🙂

PS: Actually this works because you can pass any random set of characters as a filter, and the engine will just ignore that unrecognized filter and move on to the next. It has nothing to do with # being a tag. You can try | lorem ipsum dolor sit amet | append: 'that works'

Interestingly, this may allow us to add comments anywhere already by using unrecognized filter as a hack: assign i = i + 1 | incrementing the variable or for item in cart.items | iterating over cart items 🤷 (not sure about the second example though).

Update: It works as I thought it would. But you can't use let's because the apostrophe seems to wreck it.

[dylanahsmith]

Closing in favour of #1498

[dylanahsmith]

Closing in favour of #1498