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

Add map node #11

Merged
merged 7 commits into from
Feb 16, 2017
Merged

Add map node #11

merged 7 commits into from
Feb 16, 2017

Conversation

djbe
Copy link
Member

@djbe djbe commented Jan 16, 2017

From the code gen PR: SwiftGen/SwiftGen#188

@AliSoftware AliSoftware force-pushed the master branch 2 times, most recently from 9625443 to e9e33c6 Compare January 22, 2017 16:09
AliSoftware added a commit that referenced this pull request Jan 22, 2017
@AliSoftware
Copy link
Contributor

AliSoftware commented Jan 22, 2017

This Node will be really useful to a lot of templates!

But I wonder about the syntax, still. Should the name of the variable to store the result of the map really be part of the Map node?

I mean, that node syntax reads strange to be and doesn't seem that natural to use:

{% map array with element set varname %}

I always wonder what element and varname are, which is which, etc). And the node doesn't produce any output as a result, storing the result in the context instead, that seems convoluted at first read. Especially the fact that the name of the variable to store the result is given… as the last parameter — {% set varname to map array with x %}…{% endmap %} would read more nicely to me, but starting this node with the set keyword doesn't make it clear that it's a map either and doesn't make it simple for it to be a dedicated node separate from the SetNode

That would be awesome if it would be possible to just use:

{% map array with x %}Item {{maploop.counter}} = {{x}}.{% endmap %}

And that would just generate / output… an array (rendered as a joined string if rendered directly, or stored as an array if used with a SetNode or MacroNode. That the user can then just use as-is (let it render), or store in a variable using the SetNode/MacroNode

{% set result %}{% map array with x %}Item {{maploop.counter}} = {{x}}.{% end map %}{% endset %}

Sadly, I know that's not really possible as for now, because render outputs a String (which is kinda logical), not an Array or Any.
And making it a MapFilter instead of a MapNode solve the problem either, because we need to let us pass a whole Stencil tree as the "closure" of that "map" — so {{ array|map:what_to_put_here }} wouldn't cut it (except… if what_to_put_here was a CallableNode from a MacroNode, heh!).

But I'm wondering if we couldn't find a more elegant solution that this {% map array with item set result %} or come up with a nicer and more understandable syntax?

\cc @kylef thoughts?

@djbe
Copy link
Member Author

djbe commented Jan 22, 2017

That's one of the reasons why I ended up creating a modified set that accepts a 2nd argument instead of a body, to avoid losing type information.

What we can do is simplify the map syntax of map to:
{% map array into result %}{{maploop.item|lowercase}}{% endmap %}
Not necessarily removing the option of naming the loop variable, but making it optional.

When the loop item name isn't specified we can either:

  • provide a default name such as item
  • only store the value inside maploop.item

And still allow the user to provide a name using the with myVarName arguments.

@AliSoftware
Copy link
Contributor

I like that {% map array into result [using var] %} much better than {% map array with var set result %} indeed. More understandable imho. And if the optional using var part isn't provided, maploop.item seems the best option to me (to avoid potential collision with any item variable that could have been set outside of the map and better scope everything)

@djbe
Copy link
Member Author

djbe commented Jan 22, 2017

Cool! I'll make those changes then.

CHANGELOG.md Outdated
@@ -8,6 +8,11 @@

### New Features

* Aded `MapNode` to apply a `map` operator to an array.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Aded/Added/

let result = try! template.render(MapNodeTests.context)

let expected = Fixtures.string(for: "map-basic.out")
XCTDiffStrings(result, expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if testing Stencil Nodes using templates is a good thing.
When comes the time when we decide to move the MapNode to Stencil itself, Stencil won't have our fixtures as templates to test anyway.

The way Stencil tests its nodes is by invoking the Node constructor, then its render method, and test the result, without needing a template fixture for node unit tests. See here. Maybe we should test at the same level too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look at the Stencil tests.

I'll have to try and convert the current tests for this (and other nodes) to comparable tests using constructors and such.

@djbe djbe force-pushed the feature/map-node branch 2 times, most recently from 43392b6 to 7c5687d Compare January 28, 2017 15:00
djbe pushed a commit that referenced this pull request Jan 28, 2017
@djbe djbe force-pushed the feature/map-node branch from 7c5687d to afb1eca Compare January 28, 2017 21:37
djbe pushed a commit that referenced this pull request Jan 28, 2017
@djbe djbe removed the status: WIP label Jan 29, 2017
@djbe djbe force-pushed the feature/map-node branch from 0665bf6 to 92175cb Compare January 30, 2017 17:21
djbe pushed a commit that referenced this pull request Jan 30, 2017
@djbe djbe modified the milestone: 1.0.0 Jan 31, 2017
djbe pushed a commit that referenced this pull request Feb 13, 2017
djbe pushed a commit that referenced this pull request Feb 15, 2017
@@ -19,6 +19,9 @@ _TODO: [Write more extension Documentation](https://github.com/SwiftGen/StencilS
* `SetNode`
* `{% set <Name> %}…{% endset %}`
* Renders the nodes inside this block immediately, and stores the result in the `<Name`> variable of the current context.
* `MapNode`
* `{% map <Variable> into <Name> using <ItemName> %}…{% endmap %}`
* Apply a `map` operator to an array, and store the result into a new array variable `<Name>` in the current context.
Copy link
Contributor

@AliSoftware AliSoftware Feb 16, 2017

Choose a reason for hiding this comment

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

+ * Inside the map loop, a `maploop` special variable is available (akin to the `forloop` variable in `for` nodes). It exposes `maploop.count`, `maploop.first`, `maploop.last` and `maploop.item`.

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Add doc for maploop variable, otherwise OK to merge.

@djbe djbe merged commit d6a174d into master Feb 16, 2017
@djbe djbe deleted the feature/map-node branch February 16, 2017 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants