Added support for brackets in boolean expressions #165

ilyapuchka · 2017-12-25T19:01:03Z

Currently there is no way to alter operations precedence in boolean expression, so it can force users to use cascades of ifs. With this it should become simpler.

Expression in brackets is parsed recursively as special case of IfToken that represents sub-expression which is evaluated similar to variable token.

AliSoftware

Didn't read all the code logic thoroughly yet but here are some early feedback

AliSoftware · 2018-05-10T21:28:28Z

Sources/IfTag.swift

+      if bracketsBalance < 0 { throw TemplateSyntaxError("unbalanced brackets") }
+      return bracketsBalance
+    }) == 0 else {
+      throw TemplateSyntaxError("unbalanced brackets")


Should we give a more explicit error, like "unbalanced brackets: missing closing bracket" here and "unbalanced brackets: too many closing brackets" above when <0?

AliSoftware · 2018-05-10T21:34:04Z

Sources/IfTag.swift

    }
  }

+  static func subExpression(from index: Int, components: [String], tokenParser: TokenParser) throws -> (Expression, [String]) {


Wouldn't it be better performance and memory-wise to use an ArraySlice here for components, to prevent rebuilding a new sub-array from the full array on every recursion level? After all, that's exactly what ArraySlices are designed for 😉

That would allow you to get rid of Array(…) wrapping 2 lines below too.
Of course you'd need to replace your components.enumerated() on line 133 by zip(components, components.indices) instead, which is generally better anyway (see Soroush's article about enumerated) because not all startIndices are 0.

Can't say anything about performance, but the only thing it will really change is move suffix to caller site, because IfExpressionParser accepts Array. But it allows to remove one function parameter, so I'll make the change still.

Speaking of enumeration I don't see what zip will improve in this particular case. I find enumerated use to be more clear here.

It's not about readability but about correctness (see Soroush's article), especially if we move to use slices (because they're the type to use when you recurse on subexpression, as they avoid a ton of memory reallocation, and that's exactly what we're doing here so that's a textbook use case) and enumerated will lead to wrong indexes with slices unlike zip (as explained by Soroush)

Ok, if this type only accepts ArraySlice in constructor it makes sense 👍

ilyapuchka · 2018-05-12T23:22:45Z

@AliSoftware better now?

AliSoftware

LGTM overall, but just some nitpicks, and one idea/suggestion

AliSoftware · 2018-05-12T23:36:51Z

Sources/IfTag.swift

+      var bracketsBalance = $0
+      if $1 == "(" { bracketsBalance += 1 }
+      else if $1 == ")" { bracketsBalance -= 1 }
+      if bracketsBalance < 0 { throw TemplateSyntaxError("unbalanced brackets: too many closing brackets") }


Sorry to be nitpicking about code style, but if we soon add SwiftLint to harmonize the code style of all contributors it will raise a violation here probably: even for one-liners like that it'll suggest to go to the next line after opening brace and before closing brace. Besides just being strict with style (which I agree can be annoying sometimes) this has the advantage of allowing to debug step by step, entering inside the scope of needed during debug.

AliSoftware · 2018-05-12T23:37:39Z

Sources/IfTag.swift

    }
  }

+  static func subExpression(from components: ArraySlice<String>, tokenParser: TokenParser) throws -> (Expression, Int) {


Any reason why this isn't private?

AliSoftware · 2018-05-12T23:51:40Z

Sources/IfTag.swift

+        else if $0 == ")" { bracketsBalance -= 1 }
+        return bracketsBalance != 0
+      })
+


Not really sure it would be worth it, but what about a small class Balancer helper for every time we have to balance parentheses and/or brackets? Could serve here, in the code above too, and in the other PR from @djbe about indirect references…

class Balancer { var balance = 0 let opening: Character let closing: Character init(pair: (Character, Character), initialBalance: Int = 0) { opening = pair.0 closing = pair.1 balance = initialBalance } // convenience constructors static func brackets(initial: Int = 0) -> Balancer { return Balancer(pair: ("[", "]"), initialBalance: initial) } // And same for parentheses func parse(_ char: Character) throws -> Int { // … parse the char, increment/decrement the balance if it matches the opening/closing chars, throws if balance < 0, return new balance } }

For example here you could then create a balancer then use components.prefix(while: { balancer.parse($0) != 0)

(all that code typed from my phone so no guarantee of being correct or typo-free)

I'll let you discuss if it's worth adding it? And if so adapt the code (what I typed is just a draft, maybe you'll need to adjust it to fit all situations where it could be used) but could be nice for various places where we do that kind of logic, in this PR and others

Good idea but I don't think it will be useful in #215 as validation there is done during parsing, not as a separate step. We can do the same here and remove validation step.

AliSoftware · 2018-05-12T23:51:58Z

Sources/IfTag.swift

+    }
+  }
+
+  private init(components: ArraySlice<String>, tokenParser: TokenParser) throws {


djbe

Only some tiny style changes, otherwise this LGTM.

djbe · 2018-07-11T22:43:05Z

CHANGELOG.md

@@ -7,6 +7,7 @@
 - Added support for resolving superclass properties for not-NSObject subclasses
 - The `{% for %}` tag can now iterate over tuples, structures and classes via
  their stored properties.
+- You can now use brackets in boolean expressions to change operators precedence


Could you update the entry format to match the rest? (., 2 spaces, credit yourself and ref. this PR)

djbe · 2018-07-11T22:44:06Z

Sources/IfTag.swift

+        parsedComponents.formUnion(Set(index...(index + parsedCount)))
+        return .subExpression(expression)
+      }
+      else if component == ")" {


Why isn't this on the same line as the }?

djbe · 2018-07-11T22:47:23Z

Sources/IfTag.swift

+    var bracketsBalance = 1
+    let subComponents = components
+      .prefix(while: {
+        if $0 == "(" { bracketsBalance += 1 }


Not a big fan of having the content of the if on the same line, and pretty sure swiftlint'll complain about this too.

djbe · 2018-07-11T22:50:23Z

Sources/IfTag.swift

+        parsedComponents.insert(index)
+        if let op = findOperator(name: component) {
+          switch op {
+          case .infix(let name, let bindingPower, let cls):


I had to look up what the cls value was in the Operator enum. Maybe ..., let operator woud be clearer?

djbe · 2018-07-11T22:52:19Z

Tests/StencilTests/ExpressionSpec.swift

+            .toThrow(TemplateSyntaxError("'if' expression error: infix operator 'and' doesn't have a left hand side"))
+        }
+      }
+


unneeded empty new line

djbe · 2018-07-11T23:41:54Z

Before I forget: This PR should document the changes in the docs.

ilyapuchka · 2018-08-12T19:51:33Z

@djbe comments addressed, can you look again?

# Conflicts: # CHANGELOG.md # Sources/IfTag.swift

ilyapuchka · 2018-09-11T18:23:59Z

@djbe @kylef @yonaskolb can we move this forward?

djbe · 2018-09-11T22:32:11Z

docs/builtins.rst

@@ -149,6 +149,19 @@ Will be treated as:

    one or (two and three)

+You can use brackets to change operators precedence. For example:


You can use parentheses to change operator precedence. For example:

djbe · 2018-09-11T22:34:32Z

CHANGELOG.md

@@ -11,6 +11,9 @@
 - Now requires Swift 4.1 or newer.  
  [Yonas Kolb](https://github.com/yonaskolb)
  [#228](https://github.com/stencilproject/Stencil/pull/228)
+- You can now use brackets in boolean expressions to change operators precedence.  


Weird that I didn't catch this before. Generally "brackets" (in en-US) is used to refer to "[ ]", whereas "parentheses" is used for "( )". Also pretty sure that in this situation it's "operator" (without an s).

You can now use parentheses in boolean expressions to change operator precedence.

ilyapuchka · 2018-09-21T17:31:15Z

@djbe fixed docs, good catch with brackets vs parentheses 👍

djbe · 2018-09-21T17:42:15Z

Does this still need a rebase? (github says it needs an update)

ilyapuchka · 2018-09-21T17:42:51Z

@djbe I'll update and squash

ilyapuchka requested a review from kylef December 25, 2017 19:01

ilyapuchka force-pushed the if-expression-brackets branch 2 times, most recently from 943e064 to b269783 Compare December 26, 2017 14:09

added support for brackets in boolean expressions

c7059c9

ilyapuchka force-pushed the if-expression-brackets branch from b269783 to c7059c9 Compare January 1, 2018 15:09

kylef added the enhancement label Apr 17, 2018

ilyapuchka mentioned this pull request May 10, 2018

Subscript syntax for Variables #215

Merged

ilyapuchka requested review from AliSoftware and djbe May 10, 2018 21:02

AliSoftware reviewed May 10, 2018

View reviewed changes

ilyapuchka added 3 commits May 12, 2018 23:52

more descriptive error messages

8d57645

use array slices

fb4a92e

added test for nested expressions

4a48cb5

AliSoftware reviewed May 12, 2018

View reviewed changes

removed brackets validation step

b489bf9

ilyapuchka force-pushed the if-expression-brackets branch from 0eb11df to b489bf9 Compare May 13, 2018 00:59

djbe requested changes Jul 11, 2018

View reviewed changes

djbe added this to the 0.12.0 milestone Jul 11, 2018

ilyapuchka added 3 commits August 12, 2018 20:42

Merge branch 'master' into if-expression-brackets

05b8dcd

address code review comments

0019e28

added doc comment

632710f

ilyapuchka added 2 commits September 11, 2018 19:22

Merge branch 'master' into if-expression-brackets

8792860

# Conflicts: # CHANGELOG.md # Sources/IfTag.swift

simplify expression spec

afa2dda

djbe reviewed Sep 11, 2018

View reviewed changes

fixed docs

d6ea014

djbe approved these changes Sep 21, 2018

View reviewed changes

Merge branch 'master' into if-expression-brackets

ed691de

ilyapuchka merged commit 2c3962a into master Sep 21, 2018

ilyapuchka deleted the if-expression-brackets branch September 21, 2018 19:07

djbe mentioned this pull request Sep 26, 2018

Boolean expression: parenthesis need to be surrounded by spaces #253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for brackets in boolean expressions #165

Added support for brackets in boolean expressions #165

ilyapuchka commented Dec 25, 2017 •

edited

Loading

AliSoftware left a comment

AliSoftware May 10, 2018

AliSoftware May 10, 2018 •

edited by djbe

Loading

ilyapuchka May 12, 2018

AliSoftware May 12, 2018

ilyapuchka May 12, 2018

ilyapuchka commented May 12, 2018

AliSoftware left a comment

AliSoftware May 12, 2018

AliSoftware May 12, 2018

AliSoftware May 12, 2018 •

edited

Loading

ilyapuchka May 13, 2018

AliSoftware May 12, 2018

djbe left a comment

djbe Jul 11, 2018

djbe Jul 11, 2018

djbe Jul 11, 2018

djbe Jul 11, 2018

djbe Jul 11, 2018

djbe commented Jul 11, 2018

ilyapuchka commented Aug 12, 2018

ilyapuchka commented Sep 11, 2018

djbe Sep 11, 2018

djbe Sep 11, 2018

ilyapuchka commented Sep 21, 2018

djbe commented Sep 21, 2018

ilyapuchka commented Sep 21, 2018 •

edited

Loading

		@@ -149,6 +149,19 @@ Will be treated as:

		one or (two and three)

		You can use brackets to change operators precedence. For example:

Added support for brackets in boolean expressions #165

Added support for brackets in boolean expressions #165

Conversation

ilyapuchka commented Dec 25, 2017 • edited Loading

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware May 10, 2018 • edited by djbe Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyapuchka commented May 12, 2018

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware May 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe commented Jul 11, 2018

ilyapuchka commented Aug 12, 2018

ilyapuchka commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyapuchka commented Sep 21, 2018

djbe commented Sep 21, 2018

ilyapuchka commented Sep 21, 2018 • edited Loading

ilyapuchka commented Dec 25, 2017 •

edited

Loading

AliSoftware May 10, 2018 •

edited by djbe

Loading

AliSoftware May 12, 2018 •

edited

Loading

ilyapuchka commented Sep 21, 2018 •

edited

Loading