Shopify / Shopify/liquid

Cycle context variables

Open
#1,519 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

BreaksCompatibility
Dominant language
Ruby
Stars
11.9k
Forks
1.5k
Avg merge
17h 55m
Merged PRs (30d)
3

Description

Noting that documentation for the cycle tag only mentions cycling through lists of strings, and existing integration tests cover cycling through lists of integers, is the behaviour demonstrated by the following example considered a bug?

require 'liquid'

template = Liquid::Template.parse(
  "{% cycle a, b, c %} {% cycle a, b, c %} {% cycle a, b, c %}")

puts(template.render!({"a" => 1, "b" => 2, "c" => 3}))

Output:

1 1 1

Expected Output:

1 2 3

I can see that the cycle tag was changed with this commit to parse cycle arguments as expressions, rather than treating them as strings. It appears that a string representation of objects returned from parse_expression() doesn't make a good context key.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the reproduction in the issue and the cycle coverage in test/integration/tags/standard_tag_test.rb around line 259. Trace how cycle arguments are parsed and used as context keys, then compare the observed and expected outputs. Done means the intended behavior is established and covered by an appropriate regression test.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
49/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.