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 empty to behave like an empty array (change needed in liquid-c) #1084

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

er1
Copy link
Contributor

@er1 er1 commented Apr 1, 2019

This allows the empty literal to act as an empty array when being used outside of comparisons.
It would make empty array constructions such as {% assign x = '' | split: '' %} more readable as {% assign x = empty %}
This should not impact existing uses since empty array will get coerced into empty string if and where it is being used.

Some tests are failing and depend on: Shopify/liquid-c#131

@ashmaroli
Copy link
Contributor

@er1 This looks like a breaking-change to me. Can you test if the empty array can be filled later on..?

{% assign dropship = empty %}
{% if product.price < store.sale_price %}
  {{ dropship | push: product }}
{% endif %}

@er1
Copy link
Contributor Author

er1 commented Apr 5, 2019

@ashmaroli It would depend on how that filter works. I am assuming that values are immutable and that the standard filters coerce to arrays when they need arrays before performing operations. This would be in line with how enumerable drops behave.

@er1 er1 force-pushed the method-literals branch 2 times, most recently from e145273 to f3d17c8 Compare February 11, 2020 07:59
Comment on lines +57 to 64
def test_empty_can_be_filled
template = Template.parse("{% assign foo = empty %}{{ foo | concat: bar }}")
assert_equal('Y', template.render!('bar' => ['Y']))
template = Template.parse("{% assign foo = empty %}{{ foo | concat: bar | join: '--' }}")
assert_equal('A--B', template.render!('bar' => ['A', 'B']))
template = Template.parse("{% assign foo = empty %}{% assign tar = foo | concat: bar %}{{ tar | join: '--' }}{{ tar | size }}")
assert_equal('A--B2', template.render!('bar' => ['A', 'B']))
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashmaroli would using the concat filter here be enough to check if arrays from the empty literal can be filled?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is sufficient.

@ashmaroli
Copy link
Contributor

@er1 IMO, {% assign x = empty %} doesn't improve readability. It isn't clear if x is an empty string or an empty array. It will take time for existing users to adjust to the fact that empty is a flexible entity now.
Would it be possible to introduce [] or empty_array instead..?

@er1
Copy link
Contributor Author

er1 commented Feb 11, 2020

Would it be possible to introduce [] or empty_array instead..?
@ashmaroli I agree that it looks a little unclear at first glance however I don't feel comfortable suggesting new literal names into the language.
Existing code might have empty_array set via assigns and introducing a new literal this way will break that code since literals cannot be assigned.
Using [] doesn't doesn't feel right unless we flat out introduce array literals into the language as I think it would suggest that something like [2,3,5] or [product1, product2] could be done and that is a much large discussion to have.

@ashmaroli
Copy link
Contributor

Using [] doesn't doesn't feel right unless we flat out introduce array literals into the language

I see. Thanks for the feedback.

@er1
Copy link
Contributor Author

er1 commented Nov 26, 2020

failing tests in df49715 depend on Shopify/liquid-c#131

@er1 er1 changed the title Allow method literals to provide useful values Allow empty to behave like an empty array (change needed in liquid-c) May 31, 2022
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