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

render template name with Liquid Syntax errors #1695

Merged
merged 1 commit into from Mar 2, 2023

Conversation

ggmichaelgo
Copy link
Contributor

@ggmichaelgo ggmichaelgo commented Mar 2, 2023

What are you trying to solve?

When there is a syntax error from a template, the Liquid error message would have a wrong filename:
Liquid:

# index
{% render 'snippet' %}

# snippet
1
2
{{ "hello world!" 

Error

Liquid syntax error (index line 3): Variable '{{' was not properly terminated with regexp: /\\}\\}/

How are you solving this?

In PartialCache, rescue Liquid errors from a Liquid#parse and assign template_name to it.

After:

Liquid syntax error (snippet line 3): Variable '{{' was not properly terminated with regexp: /\\}\\}/

@ggmichaelgo ggmichaelgo force-pushed the syntax-error-with-filename branch 4 times, most recently from e4ecca4 to 49d5b36 Compare March 2, 2023 20:11
@ggmichaelgo ggmichaelgo marked this pull request as ready for review March 2, 2023 20:11
partial.name ||= template_name
begin
partial = template.parse(source, parse_context)
partial.name ||= template_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: This line could be moved after the rescue to make it clearer that template.parse is the only thing we need the rescue for.

)
end

def test_syntax_error_is_raised_with_actual_filenmae
Copy link
Contributor

Choose a reason for hiding this comment

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

There is at least a typo here in filename.

Also, with_actual_filename isn't really accurate, since it isn't the actual on disk file path (not does liquid itself even know that). You use with_template_name in a following test which seems more appropriate

Suggested change
def test_syntax_error_is_raised_with_actual_filenmae
def test_syntax_error_is_raised_with_template_name

)
end

def test_stack_level_error_is_raised_with_actual_filenmae
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_stack_level_error_is_raised_with_actual_filenmae
def test_stack_level_error_is_raised_with_template_name

@ggmichaelgo ggmichaelgo merged commit 48cb643 into master Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants