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

Only use MethodLiteral in condition expressions #1300

Merged
merged 2 commits into from
Sep 25, 2020

Conversation

dylanahsmith
Copy link
Contributor

@dylanahsmith dylanahsmith commented Sep 24, 2020

Problem

The MethodLiteral literal value was added in #592 which described it as a hacky solution:

Not sure if this is a good fix, feels kinda hacky, but seems to work.

One way in which it is hacky is that it is only relevant for boolean comparison equality expressions and isn't relevant for all expressions.

In liquid-c (Shopify/liquid-c#60), I would like Liquid::Expression.parse to return a Liquid::C::Expression object that can be rendered more efficiently and I would like to avoid coupling this work to these MethodLiteral objects.

Solution

Add a Liquid::Condition.parse_expression method that wraps Liquid::Expression.parse so that it will return a MethodLiteral object for empty or blank literals and will otherwise delegate to Liquid::Expression.parse. This way the values for these literals that are returned by Liquid::Expression.parse can just be the empty strings that MethodLiteral#to_liquid returns.

Copy link
Member

@tjoyal tjoyal left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants