Subscript syntax for Variables by djbe · Pull Request #215 · stencilproject/Stencil

djbe · 2018-05-10T20:22:27Z

This allows users to refer to variables (and properties) using indirection.

Let's say for example we have the following context:

[
  "object": [
    "name": "John",
    "role": "Developer",
    "location": "Somewhere"
  ],
  "visible": ["name", "role"]
]

A user can then write:

{% for property in visible %}
 - {{property}}: {{ object.$property }}
{% endfor %}

To produce:

- name: John
- role: Developer

djbe · 2018-05-10T20:24:51Z

Note: the reason I went with the object.$property syntax instead of something like object[property] is that there's no cross-platform regex support (NSRegularExpression is macOS only).

ilyapuchka · 2018-05-10T20:27:38Z

Sources/Variable.swift

      current = normalize(current)

+      // try to evaluate bit if it's an indirection
+      var resolvedBit = bit


you can do for var bit and mutate bit itself

ilyapuchka · 2018-05-10T20:30:18Z

Sources/Variable.swift

+      var resolvedBit = bit
+      if bit.hasPrefix("$"),
+        let resolved = try? Variable(String(bit.dropFirst())).resolve(context),
+        let property = resolved {


maybe better to do resolved as? String?

I had that first, but things break then if the resolved value is an Int for example (see the tests with array indirect lookup)

AliSoftware · 2018-05-10T20:35:16Z

I love the idea.

It's a shame that we can't use subscript for that though

FWIW I think you could be able to use .[prop] instead of .$prop even without using regex, eg:

if bit.hasPrefix("[") && bit.hasSuffix("]"),
       let resolved = try? Variable(String(bit.dropFirst().dropLast())).resolve(context),
…

but even with such a solution that would still mean the syntax need to use the dot, and would look like object.[property] where what we would ideally want would be object[property]. So in that regard if the solution needs for technical reasons to use a dot then I prefer o.$p to o.[p] and if we want to support o[p] we'd have to find a different solution for the implementation
Before validating that kind of syntax I'd suggest to look at the syntax Jinja uses for such feature (if it does support it, that is). We have to remember that Stencil is inspired by Jinja-like template engines and it's better for the end user of we can use the same or very similar syntax and tags whenever possible/relevant for similar features.

djbe · 2018-05-10T20:37:06Z

Jinja uses braces for lookup:
https://stackoverflow.com/questions/27208891/jinja2-dictonary-lookup-using-a-variable-key#27208914

AliSoftware · 2018-05-10T20:39:41Z

Damn so that would still be better to find a way to do that with braces without dot, both because it feels more natural and because it's similar to Jinja… but I guess you tried to find a solution for that already?

djbe · 2018-05-10T20:40:38Z

I'm honestly not sure how to tackle that without regular expressions, and without blowing this code up into some massive thing.

AliSoftware

The fact that this solution uses o.$p and not o[p] also means that p can't be a compound expression 😢

So we can't express object[property.name|lowercase] for example with the $ syntax (except by using an intermediate {% set %} but that's not as pretty then)

ilyapuchka · 2018-05-10T20:49:10Z

I didn't test this code, but this might work:

  fileprivate func lookup() -> [String] {
    return variable.split(separator: ".")
        .flatMap({ $0.split(separator: "[", maxSplits: 1) })
        .map({ String($0).trim(character: "]") })
  }

This will though only work on single brackets, like foo[bar], but not foo[bar[abc]], which I'm not sure we should even support.

@AliSoftware good point about filter.
Simple alternative to all of that can be a filter like foo|attr:bar (or with any other maningful name)

ilyapuchka · 2018-05-10T20:52:55Z

Can also try split(maxSplits: 2, whereSeparator isSeparator: { $0 == "[" || $0 == "]" }) variant where separator can be either [ or ] so there will be no need for trim, I guess.

AliSoftware · 2018-05-10T20:53:48Z

@ilyapuchka Mmmh interesting idea to use a filter to extract the property. But that would still mean we couldn't apply a filter on that filter 😅
it would enable us to do object|attr:property.name to do the equivalent of Jinja's object[property.name] but we still wouldn't be able to do the equivalent of Jinja's object[property.name|lowercase] for example

AliSoftware · 2018-05-10T20:55:23Z

I really think the right syntax is to use the same syntax as Jinja, the question is how to implement it in Stencil without breaking all the code…

ilyapuchka · 2018-05-10T20:56:36Z

@AliSoftware true. We can for that add support for brackets in filter expressions item|attr:(bar|attr:abc) which might be also useful in general. I have added that for boolean expressions in #165

djbe · 2018-05-10T20:56:49Z

I'm working on a solution right now, might take a bit. I'm going to iterate over the characters (in the lookup function), and keep track if I'm in a lookup (between [ and ]). I'll even support multiple lookup levels 😄

kylef · 2018-05-10T21:07:32Z

CHANGELOG.md

  [Yonas Kolb](https://github.com/yonaskolb)
-  [#394](https://github.com/stencilproject/Stencil/pull/214)
+  [#214](https://github.com/stencilproject/Stencil/pull/214)
+- Variables now support indirect evaluation. For example, if you have a variable `key = "name"`, and an


This would be a breaking change right? If so then it should go into ### Breaking section too.

For example, the following that works in Stencil 0.11 would no longer work?:

let schema = [ "$schema": "http://json-schema.org/draft-06/schema#", "type": "object", "description" "A geographical coordinate", "properties": [ "latitude": ["type": "number"], "longitude": ["type": "number"], ] ]

<h2>{{ schema.description|default:"Schema" }}</h2> <a href="{{ schema.$schema }}">View Schema Specification</a> <code> {{ schema|serialiseJSON }} </code>

Perhaps an alternative would be to make this a filter itself instead of overloading special sub-syntax into this, then the same can be achieved by the following syntax which perhaps it a little clearer for the reader of the template:

{{ item|valueForKey:"key" }}

We had a pr for that actually #191, but then I added support for single parameter for map filter in #189 , for which we can also define valueForKey synonym.

Yeah, I'm rewriting it to use object[property] syntax like jinja.

djbe · 2018-05-10T22:15:35Z

Since the force push is hiding the commit comments:

Does that mean that object[[[]]]...[]..[] would be valid, as you filter out empty bits?

We should define the syntax a bit more. To make things a bit easer:

should {{ [property] }} be valid? (see first test, it should IMO)
if it is, then {{ object[[[stuff]]] }} is valid
the .... was already valid in Stencil.

ilyapuchka · 2018-05-12T20:41:41Z

Sources/LookupParser.swift

+    }
+
+    guard !partialBits.isEmpty else {
+      throw TemplateSyntaxError("Attempting to dereference empty object in variable '\(variable)'")


What does this mean? Maybe we can say the same simpler? I guess this handles .. case? The we should just say that this is invalid syntax.

It's more than just .., it could be stuff.., or stuff..other.stuff, or other cases. It's already a TemplateSyntaxError, this just specifies what's wrong.

If I understand code flow correctly here errors will all fall into unexpected '.' in \(variable) kind of description.

ilyapuchka · 2018-05-12T20:44:14Z

Sources/LookupParser.swift

+struct LookupParser {
+  private var bits = [String]()
+  private var current = ""
+  private var partialBits = [String]()


What partialBits stands for?

In a lookup of my[a.b].stuff[ref][ref2].here, the parser will go through every bit, which might contain bracket syntax. So in this case, the partialBits would end up being:

my[a.b]

stuff[ref][ref2]

here

It's called bit because the result is a bunch of bits (old name, from before this PR), and partial, because it's still parsing the bit. Naming things is hard, and it's not like stuffBeforeDot is better?

ilyapuchka · 2018-05-12T20:45:24Z

Sources/LookupParser.swift

+
+/// A structure used to represent a template variable, and to resolve it in a given context.
+struct LookupParser {
+  private var bits = [String]()


Maybe rename it to keyPath?

That'd be more a question for @kylef, what's the reason for the bits naming?

ilyapuchka · 2018-05-12T20:46:21Z

Sources/LookupParser.swift

+  // when opening the first bracket, we must have a partial bit
+  private mutating func openBracket() throws {
+    guard !partialBits.isEmpty || !current.isEmpty else {
+      throw TemplateSyntaxError("Attempting to dereference an empty object in variable '\(variable)'")


same as before, if this handles [[ syntax then let's just say that in error message

Same answer as before. [[ might be just one of the cases, we can't assume that.

And here it can be probably unexpected '[' in '\(variable)'

ilyapuchka · 2018-05-12T20:50:38Z

Sources/LookupParser.swift

+import Foundation
+
+/// A structure used to represent a template variable, and to resolve it in a given context.
+struct LookupParser {


Maybe VariableLookup will be a better name? I would say that's the main purpose of this type where parsing is secondary.

Also probably this can be a class, so you can remove mutating attribute (which will also probably save from unnecessary memory allocations in runtime)

Naming things is hard. Maybe KeypathParser, using your keypath suggestion from before?
Input from others is welcome, so we can reach a consensus on this.

ilyapuchka · 2018-05-12T21:02:15Z

Tests/StencilTests/VariableSpec.swift

+        try context.push(dictionary: ["property": 5]) {
+          let variable = Variable("contacts[property]")
+          let result = try variable.resolve(context) as? String
+          try expect(result).to.beNil()


it is probably unrelated to this PR, but I wonder if it would be better to raise runtime exceptions in such cases, I find it strange that Stencil just returns nils in such cases, it can be really hard to debug templates with such behaviour. I.e. I think EJS will raise exception in this case, but I don't know what is the common approach in other templating languages.

I think django/jinja just return nils, like here. It's up to the developer to check for empty cases and handle these accordingly. How would you otherwise check if something exists or not, without triggering an exception?

I see it like that. Lookup function should return nil if variable is not defined in the context, but if it is array subscript or type introspection it should throw. But even in the first case I don't think nil lookup result should be rendered as empty strings, it should be exception. In Jinja I believe one can check if variable is not nil using is defined ({% if loop.previtem is defined and value > loop.previtem %}). But I'm not very well familiar with Jinja and Django to advocate for that and it is not related to this PR so lets leave it for another time.

Quote from Jinja template docs:

If a variable or attribute does not exist, you will get back an undefined value. What you can do with that kind of value depends on the application configuration: the default behavior is to evaluate to an empty string if printed or iterated over, and to fail for every other operation.

So it returns an invalid value (doesn't throw), even for subscripts, that can be printed, iterated (and tested in ifs I assume), but other operations will fail (exception) for it.

ilyapuchka · 2018-05-12T21:03:18Z

Tests/StencilTests/VariableSpec.swift

+        }
+      }
+
+      $0.it("can resolve multiple indirections") {


copy paste?

ilyapuchka · 2018-05-12T21:04:08Z

Tests/StencilTests/VariableSpec.swift

+        }
+      }
+
+      $0.it("can resolve multiple indirections") {


I think in implementation you call it as references, I guess it should be used here too

I'm not sure if it should be called indirections, references, subscripts, ... 😕

For those with Swift background the most clear name would be probably subscript.

ilyapuchka · 2018-05-12T21:04:20Z

Tests/StencilTests/VariableSpec.swift

+        }
+      }
+
+      $0.it("can resolve recursive indirections") {


nested references?

ilyapuchka · 2018-05-12T21:05:25Z

Tests/StencilTests/VariableSpec.swift

+            "article.author[prop]]",
+            "article.author[]",
+            "article.author[[]]",
+            "article.author[prop][]",


also article.author[prop]comments (missing . after ])

ilyapuchka

I think this is awesome 👏

djbe added the enhancement label May 10, 2018

ilyapuchka approved these changes May 10, 2018

View reviewed changes

djbe force-pushed the feature/variable-indirection branch from 03cf1db to efc5ad3 Compare May 10, 2018 20:43

AliSoftware reviewed May 10, 2018

View reviewed changes

kylef reviewed May 10, 2018

View reviewed changes

djbe force-pushed the feature/variable-indirection branch from 258e876 to 3e6bbf2 Compare May 10, 2018 22:11

ilyapuchka reviewed May 12, 2018

View reviewed changes

ilyapuchka mentioned this pull request May 13, 2018

Added support for brackets in boolean expressions #165

Merged

djbe added 9 commits May 13, 2018 20:57

Implement variable indirect resolution

6bb87fc

Add some tests

db51150

Changelog entry

14d6fec

Update documentation

b44812c

Rework the syntax to use brackets instead of a $

5343388

Move the lookup parser into it's own file

f5a2eb3

Add invalid syntax tests

9c7e3d4

Swift 3 support

0157c89

Rename some things + extra test

e395a10

djbe force-pushed the feature/variable-indirection branch from 8d76b79 to e395a10 Compare May 13, 2018 19:25

djbe changed the title ~~Variable indirection~~ Subscript syntax for Variables May 13, 2018

djbe mentioned this pull request May 14, 2018

New Command: Yaml and plist support SwiftGen/SwiftGen#379

Merged

ilyapuchka approved these changes May 19, 2018

View reviewed changes

ilyapuchka merged commit 2e18892 into master May 19, 2018

ilyapuchka deleted the feature/variable-indirection branch May 19, 2018 20:03

djbe added this to the 0.12.0 milestone Jul 12, 2018

AliSoftware mentioned this pull request Aug 24, 2018

New Command: Add Support for Core Data SwiftGen/SwiftGen#455

Merged

Conversation

djbe commented May 10, 2018

Uh oh!

djbe commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djbe commented May 10, 2018

Uh oh!

AliSoftware commented May 10, 2018

Uh oh!

djbe commented May 10, 2018

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

ilyapuchka commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilyapuchka commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented May 10, 2018

Uh oh!

AliSoftware commented May 10, 2018

Uh oh!

ilyapuchka commented May 10, 2018

Uh oh!

djbe commented May 10, 2018

Uh oh!

kylef May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djbe May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djbe commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AliSoftware commented May 10, 2018 •

edited

Loading

ilyapuchka commented May 10, 2018 •

edited

Loading

ilyapuchka commented May 10, 2018 •

edited

Loading

kylef May 10, 2018 •

edited

Loading

djbe May 10, 2018 •

edited

Loading