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 Swift 5.2 support #125

Closed
wants to merge 2 commits into from
Closed

Conversation

howlingblast
Copy link

Change Log

  • Fix a crash due to order of conditions
  • Change dependency of the Stencil package to the master branch; this is not optimal, but Stencil didn't have a release for a long time and Swift 5.2. support is only available on the master branch
  • Adapt code to Swift 5.2/Stencil master (API changes)

Copy link
Member

@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.

As mentioned in comments, It's a bad idea to depend on master, I'll see about making a Stencil release. Missing from this PR is an update to the podspec/Podfile.

@@ -95,7 +95,7 @@ extension Filters.Strings {
while idx < scalars.endIndex, let scalar = UnicodeScalar(scalars[idx].value), characterSet.contains(scalar) {
idx = scalars.index(after: idx)
}
if idx > scalars.index(after: start) && idx < scalars.endIndex,
if idx < scalars.endIndex, idx > scalars.index(after: start),
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this fixes the crash?

@@ -79,7 +79,7 @@ class CallNodeTests: XCTestCase {
}

func testRenderFail() {
let context = Context(dictionary: [:])
let context = Context(dictionary: ["": ""])
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? (same for the lines below)

@@ -7,7 +7,7 @@ let package = Package(
.library(name: "StencilSwiftKit", targets: ["StencilSwiftKit"])
],
dependencies: [
.package(url: "https://github.com/stencilproject/Stencil.git", .upToNextMinor(from: "0.13.0"))
.package(url: "https://github.com/stencilproject/Stencil.git", .branch("master"))
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather avoid this if possible, I can probably push through a Stencil release if needed.

Do we have Swift 5 support in Stencil yet? (it's been a while)

@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure we should add this folder to gitignore.

@AliSoftware AliSoftware changed the base branch from master to stable June 10, 2020 23:03
@AliSoftware
Copy link
Contributor

PR #127 is pending and will soon update StencilSwiftKit to latest version of Stencil. This means we can stop pointing to the master branch in this PR, and that the changes like this one will already be that other PR.

@djbe
Copy link
Member

djbe commented Oct 9, 2020

Thank you for your contribution.

I know it's been a while, and in the mean time, this has been superseded by #127, which (amongst other things) adds support for Swift 5.

@djbe djbe closed this Oct 9, 2020
@djbe djbe added this to the 2.8.0 milestone Oct 9, 2020
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