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

<for> switch to tag params #1238

Merged
merged 6 commits into from
Jan 29, 2019
Merged

<for> switch to tag params #1238

merged 6 commits into from
Jan 29, 2019

Conversation

DylanPiercey
Copy link
Contributor

@DylanPiercey DylanPiercey commented Jan 18, 2019

Description

This pr updates the <for> tag to use tag params instead of the existing special syntax.

It also normalizes the for loop, and removes some unneeded functionality. Previously the <for> use a javascript like syntax that deviated in many ways.

Here are some of the previous ways you could write a for loop:
for(item in array)
for(item in callbackGeneratorFunction)
for(key, value in object)
for(i from 0 to 10 step 2)
for(item in array | status-var=loop)
for(item in array | separator=" ,")
for(item in array | iterator=someFunction)
for(item in array | array)

Migrators have been added in this PR for all of the above variants where possible. Where a migration was not possible there now may be some additional runtime warnings, but all existing syntax should continue to work until a future version of Marko. You can automatically migrate the old syntax using marko migrate.

The new <for> loop

The <for> loop in this PR does not introduce any additional syntax and leverages the new tag parameters syntax.

There are three variants, here are some examples of using the <for> loop:

with an of attribute (iterates over an array/iterable).

<for|item, index, all| of=array>
  $ const isLast = index === all.length - 1;
  ${index}: ${item}

  <if(!isLast)><br/></if>
</for>

with an in attribute (iterates over an objects properties).

<for|key, value| in=object>
  ${key}: ${value}
</for>

with a from and to attribute (iterates between a range of numbers).

<for|i| from=0 to=10 step=2>
  ${i}
</for>

Checklist:

  • I have read the CONTRIBUTING document and have signed (or will sign) the CLA.
  • I have updated/added documentation affected by my changes.
  • I have added tests to cover my changes.

@coveralls
Copy link

coveralls commented Jan 18, 2019

Coverage Status

Coverage decreased (-0.1%) to 90.462% when pulling 565dc73 on for-tag-params into de397d4 on tag-param-core-tags.

@codecov
Copy link

codecov bot commented Jan 18, 2019

Codecov Report

Merging #1238 into tag-param-core-tags will increase coverage by 0.03%.
The diff coverage is 89.35%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           tag-param-core-tags    #1238      +/-   ##
=======================================================
+ Coverage                90.56%   90.59%   +0.03%     
=======================================================
  Files                      337      334       -3     
  Lines                    12356    12339      -17     
=======================================================
- Hits                     11190    11179      -11     
+ Misses                    1166     1160       -6
Impacted Files Coverage Δ
src/taglibs/migrate/util/parseFor.js 94.33% <ø> (ø)
src/compiler/util/removeComments.js 100% <ø> (ø) ⬆️
src/compiler/CompileContext.js 92.28% <ø> (ø) ⬆️
src/compiler/Builder.js 92.05% <100%> (+1.39%) ⬆️
src/runtime/helper-forEachProperty.js 93.75% <100%> (+1.44%) ⬆️
src/compiler/Normalizer.js 98.33% <100%> (ø) ⬆️
src/runtime/helpers.js 94.06% <66.66%> (-4.05%) ⬇️
src/compiler/ast/ForEach.js 78.57% <66.66%> (-9.9%) ⬇️
src/taglibs/core/for-tag.js 73.91% <72.72%> (-18.4%) ⬇️
src/compiler/ast/ForEachProp.js 82.35% <75%> (-6.84%) ⬇️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de397d4...565dc73. Read the comment docs.

docs/core-tags.md Outdated Show resolved Hide resolved
${i}
</for>

<for(var i = 0; i <= listSize; i += 2)>
<!-- Stange: backwards -->
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member

@mlrawlings mlrawlings left a comment

Choose a reason for hiding this comment

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

Looks like a few cases aren't handling the status var correctly

</for>
<!-- Array | status-var iterator separator -->
<for|color, loop, loopIndex, loopAll| of=input.iterator.bind(null, colors)>
${color}${loop.getIndex() + 1}) of ${loop.getLength()}<if(loop.isFirst())>
Copy link
Member

Choose a reason for hiding this comment

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

${loop.getIndex() + 1}) of ${loop.getLength()}

${color}${loop.getIndex() + 1}) of ${loop.getLength()}<if(loop.isFirst())>
- FIRST
</if>
<if(loop.isLast())>- LAST</if>
Copy link
Member

Choose a reason for hiding this comment

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

loop.isLast()

@DylanPiercey DylanPiercey changed the base branch from master to tag-param-core-tags January 29, 2019 23:10
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