Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow # to be used as an inline comment tag #1401

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,6 @@ group :test do
gem 'rubocop-performance', require: false

platform :mri, :truffleruby do
gem 'liquid-c', github: 'Shopify/liquid-c', ref: 'master'
gem 'liquid-c', github: 'Shopify/liquid-c', ref: 'inline-comment'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

end
end
5 changes: 5 additions & 0 deletions History.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Liquid Change Log

## Unreleased

### Features
* Allow `#` to be used as an inline comment tag (#1401) [Dylan Thacker-Smith]

## 5.0.0 / 2021-01-06

### Features
Expand Down
4 changes: 2 additions & 2 deletions lib/liquid/block_body.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

module Liquid
class BlockBody
LiquidTagToken = /\A\s*(\w+)\s*(.*?)\z/o
FullToken = /\A#{TagStart}#{WhitespaceControl}?(\s*)(\w+)(\s*)(.*?)#{WhitespaceControl}?#{TagEnd}\z/om
LiquidTagToken = /\A\s*(\w+|#)\s*(.*?)\z/o
FullToken = /\A#{TagStart}#{WhitespaceControl}?(\s*)(\w+|#)(\s*)(.*?)#{WhitespaceControl}?#{TagEnd}\z/om
ContentOfVariable = /\A#{VariableStart}#{WhitespaceControl}?(.*?)#{WhitespaceControl}?#{VariableEnd}\z/om
WhitespaceOrNothing = /\A\s*\z/
TAGSTART = "{%"
Expand Down
15 changes: 15 additions & 0 deletions lib/liquid/tags/inline_comment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true

module Liquid
class InlineComment < Tag
def render_to_output_buffer(_context, output)
output
end

def blank?
true
end
end

Template.register_tag('#', InlineComment)
end
57 changes: 57 additions & 0 deletions test/integration/tags/inline_comment_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# frozen_string_literal: true

require 'test_helper'

class InlineCommentTest < Minitest::Test
include Liquid

def test_tag_in_different_styles
assert_template_result('', '{% # This text gets ignored %}')
assert_template_result('', '{%# This text gets ignored #%}')
assert_template_result('', '{%# This text gets ignored %}')
assert_template_result('', '{%#- This text gets ignored -#%}')
end

def test_test_syntax_error
assert_template_result('fail', '{% #This doesnt work %}')

assert false
rescue
# ok good
end
Comment on lines +15 to +21
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


def test_tag_ws_stripping
assert_template_result('', ' {%#- This text gets ignored -#%} ')
Comment on lines +23 to +24
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 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 -%}.

end

def test_comment_inline_tag
assert_template_result('ok', '{% echo "ok" # output something from a tag %}')
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


assert_template_result('ok', '{% # this sort of comment also
echo "ok" %}')
end

def test_comment_inline_variable
assert_template_result('ok', '{{ "ok" # output something from a variable }}')
assert_template_result('ok', '{{ "OK" | downcase # output something from a variable }}')
end

def test_inside_liquid_tag
source = <<~LIQUID
{%- liquid
echo "before("
# This text gets ignored
echo ")after"
-%}
LIQUID
assert_template_result('before()after', source)
end

def test_multiline
assert_template_result('', '{% # this sort of comment also
# will just work, because it parses
# as a single call to the "#" tag %}')

end

end