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 commas to separate for tag attributes #1658

Merged
merged 1 commit into from Nov 29, 2022

Conversation

dylanahsmith
Copy link
Contributor

Problem

There are multiple tags that use Liquid::TagAttributes to scan for attributes, but the only one that has strict parsing is the for tag and that strict parsing doesn't allow commas as separators as we typically use in other tags for tag attributes.

For example, the render tag documentation for passing variables to a snippet demonstrates this with {% render 'filename', variable: value %}. Based on what I see in the dawn theme, it looks like this is our preferred style (e.g. we have {% render 'card-collection', card_collection: block.settings.collection , media_aspect_ratio: section.settings.image_ratio, columns: columns %})

In contrast, trying to use the same style in the for tag (e.g. {% for i in array, limit: 4, offset: 2 %}...{% endfor %}) would be a syntax error in strict parsing mode, requiring only a space to be used as a separate (e.g. {% for i in array limit: 4 offset: 2 %}...{% endfor %}).

Solution

Update Liquid::For#strict_parse to optionally allow commas to separate tag attributes.

Note that the for tag also allows a reversed option to be specified, but it is different from a tag attribute in that it isn't given a value and is expected to precede any tag attributes. Given this, I decided it didn't need to also allow a comma to precede it.

For consistency with tags like the `render` tag, where we actually
prefer to use commas to separate attributes
@dylanahsmith dylanahsmith merged commit c2c6cb2 into master Nov 29, 2022
@dylanahsmith dylanahsmith deleted the for-tag-attributes-comma-sep branch November 29, 2022 14:01
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