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

Compile filters in call node #85

Merged
merged 4 commits into from
Apr 17, 2018

Conversation

ilyapuchka
Copy link
Collaborator

Resolves #76

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.

In addition to the linter violation to fix, could be nice to update the tag's doc as we're at it to add an example using one or multiple arguments using filter expression(s) to show in the doc that it's now possible 😉

@@ -38,7 +38,8 @@ class CallNodeTests: XCTestCase {
}

XCTAssertEqual(node.variableName, "myFunc")
XCTAssertEqual(node.arguments, [Variable("a"), Variable("b"), Variable("c")])
let variables = node.arguments.flatMap({ $0 as? FilterExpression }).flatMap({ $0.variable })
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally agree with this code style too when chaining flatMaps but our linter disagrees there, so maybe adapt the syntax here, like let variables = node.arguments.flatMap { ($0 as? FilterExpression)?.variable }?

@AliSoftware
Copy link
Contributor

AliSoftware commented Apr 17, 2018

I think there was another linting violation reported by the CI — but on BoolFiltersTests (which is strange because you didn't change it in that PR, maybe a leftover violation that passed though just at the time migrated to CircleCI 2 and the PR checks had to change to adapt and we missed that and let it slip, idk? would need fixing anyway so might as well fix in this PR)

@ilyapuchka
Copy link
Collaborator Author

@AliSoftware which one? I don't see errors or warnings in the lint logs

@AliSoftware
Copy link
Contributor

AliSoftware commented Apr 17, 2018

@ilyapuchka Nvm, false alarm, my bad.

The story is that I looked at the CircleCI report from my iPhone, but since CircleCI isn't really responsive and doesn't display the logs in a super-adapted way on mobile, I got confused by the console logs. The console writes Linting 'ParseBoolTests.swift' (6/12) just before the first /Users/distiller/project/Tests/StencilSwiftKitTests/CallNodeTests.swift:41:21: warning: Trailing Closure Violation: Trailing closure syntax should be used whenever possible. (trailing_closure) so I first misread that on my tiny phone screen as the violation being the result of linting ParseBoolTests.swift… 🤦‍♂️

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.

Might want to ask @gushromp (OP on #76) to test if that PR/branch fixes their issue, but looking at the changes from here on GitHub, LGTM.

CHANGELOG.md Outdated

### Bug fixes

* Fixed using filter expression in call node.
Copy link
Contributor

@AliSoftware AliSoftware Apr 17, 2018

Choose a reason for hiding this comment

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

Ah, just missed that remark though:
Missing two spaces at the end of the line after the . — for markdown formatting purposes

(we should def add a Dangerfile and rule for linting our CHANGELOG one day, just never had time to write one for our specific CHANGELOG formatting convention yet)

CHANGELOG.md Outdated
@@ -6,7 +6,7 @@

### Bug fixes

* Fixed using filter expression in call node.
* Fixed using filter expression in call node.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@AliSoftware AliSoftware merged commit c85c2d0 into SwiftGen:master Apr 17, 2018
@djbe djbe added this to the 2.6.0 milestone Oct 7, 2018
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