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

Resolvable boolean expressions #164

Closed
wants to merge 5 commits into from
Closed

Conversation

ilyapuchka
Copy link
Collaborator

@ilyapuchka ilyapuchka commented Dec 23, 2017

This PR adds ability to resolve boolean expressions the same way as variables like {{ this == that }}

This have a potential to simplify templates when there is a need to evaluate boolean expression and output its result, true or false, in a template, i.e. when generating html to enable or disable html elements. Instead of {% if this == that %}true{% else %}false{% endif %} it will be possible to write just {{ this == that }}

Also this PR adds support for parsing static boolean expressions, i.e. if {% if true %}. There was implementation for such expression and tests for it, but it was never actually parsed. It may not be very important, but can be helpful during template development.

@ilyapuchka ilyapuchka force-pushed the resolvable-expressions branch 2 times, most recently from 670e440 to 594f603 Compare December 26, 2017 13:46
@ilyapuchka ilyapuchka mentioned this pull request Jan 16, 2018
12 tasks
@ilyapuchka ilyapuchka requested a review from kylef January 23, 2018 20:34
removed static boolean expressions
added test for rendering template with boolean expression
Copy link
Contributor

@djbe djbe left a comment

Choose a reason for hiding this comment

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

Other than the changelog entry, LGTM.

CHANGELOG.md Outdated
[#178](https://github.com/stencilproject/Stencil/pull/178)

- Now boolean expressions results can be rendered, i.e `{{ name == "John" }}` will render `true` or `false` depending on the evaluation result
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot the . and 2 spaces. Also remove the empty line above.

/// Resolves a variable in the given context as boolean
func resolve(context: Context, variable: Resolvable) throws -> Bool {
func evaluate(context: Context) throws -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, I think I've seen this change a few times in other PRs? 😕😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe, some features required similar changes

@djbe
Copy link
Contributor

djbe commented Jul 11, 2018

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

@djbe djbe added this to the 0.12.0 milestone Jul 11, 2018
Copy link
Collaborator

@kylef kylef left a comment

Choose a reason for hiding this comment

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

I started reviewing this a while back, gonna submit my old review for now.

let expression = try! parseExpression(components: ["lhs", "or", "rhs"], tokenParser: parser)

let components = ["lhs", "or", "rhs"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trailing newlines

@@ -95,11 +100,17 @@ func testExpressions() {
$0.it("returns truthy for positive expressions") {
let expression = NotExpression(expression: StaticExpression(value: true))
try expect(expression.evaluate(context: Context())).to.beFalse()

try expect(Template(templateString: "{% if true %}true{% else %}false{% endif %}").render(Context())) == "true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is the unit tests for expression, I do not want to test other units in this file. This is further becoming an integration test when we are creating a template and testing other functionality such as the `if statements.

@@ -136,163 +147,172 @@ func testExpressions() {
}
}

func expectExpression(with components: [String], context: [String: Any], toBe expected: Bool) throws {
let expression = try parseExpression(components: components, tokenParser: parser)
try expect(expression.evaluate(context: Context(dictionary: context))) == expected
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will cause the source maps for the thrown error when the tests fail to be raised from here making test failures harder to pinpoint. To solve this we can grab the file and line number from the expectExpression invocation and pass it to expect:

func expectExpression(..., file: String = #file, line: Int = #line, function: String = #function) {
  ...
  try expect(..., file: file, line: line, function: function) == expected
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea!

@ilyapuchka
Copy link
Collaborator Author

@djbe @kylef comments addressed, can you look again?

@ilyapuchka ilyapuchka changed the title Resolvable expressions Resolvable boolean expressions Sep 11, 2018
@ilyapuchka ilyapuchka closed this Sep 11, 2018
@djbe
Copy link
Contributor

djbe commented Sep 13, 2018

@ilyapuchka why close this? The docs are there, it should be GTM.

@ilyapuchka
Copy link
Collaborator Author

there were conflicts and tests started to fail for some reason after I merged, I'll need to to investigate that but I also don't feel like this is a top priority considering other open PRs

@djbe
Copy link
Contributor

djbe commented Sep 13, 2018

Which tests? CI succeeds for the latest commit in this PR.

@ilyapuchka
Copy link
Collaborator Author

Only because I didn't push 😉

@djbe djbe modified the milestones: 0.12.0, 0.15.0 Jul 28, 2022
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