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.

Sign up for GitHub

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

Jump to bottom

Feat: Add support for Swift 4.0 #258

Merged
merged 4 commits into from
Oct 1, 2018
Merged

Feat: Add support for Swift 4.0 #258

merged 4 commits into from
Oct 1, 2018

Conversation

Andrew-Lees11
Copy link
Contributor

@Andrew-Lees11 Andrew-Lees11 commented Oct 1, 2018 •
edited
Loading

I am part of the Kitura team and we would like to update to the latest version of Stencil, however since we support Swift 4.0 and so are unable to use the latest version unless Stencil also supports Swift 4.0.

This pull requests adds support for Swift 4.0 by

  • adding the auto-generated functions for equatable
  • Using #if swift(>=4.1) to alternate between flatMap and compactMap
  • Using #if swift(>=4.1) to alternate the requirement for where T.IndexDistance == Int with collections

Swift 4.0.3 builds have also been added to travis to check for future Swift 4.0 is supported in future builds.

These changes allow Stencil to compile and run on Swift 4.0 while still compiling with no build warnings for Swift 4.1

ilyapuchka
ilyapuchka reviewed Oct 1, 2018
View reviewed changes
Package.swift
@@ -1,4 +1,4 @@
// swift-tools-version:4.1
// swift-tools-version:4.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it still work when built with swift 4.1?...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will still work. The swift-tools-version specifies the minimum version of Swift required for the project but higher versions are allowed.

ilyapuchka
ilyapuchka reviewed Oct 1, 2018
View reviewed changes
Sources/Errors.swift Outdated
lhs.token == rhs.token &&
lhs.stackTrace == rhs.stackTrace &&
lhs.templateName == rhs.templateName &&
lhs.allTokens == rhs.allTokens
Copy link
Collaborator

@ilyapuchka ilyapuchka Oct 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

allTokens check is not needed as it is computed property. Also, lines starting with 43 should be indented on one position.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also wrap this whole == operator in a #if swift check.

ilyapuchka
ilyapuchka reviewed Oct 1, 2018
View reviewed changes
Sources/IfTag.swift Outdated
@@ -118,7 +118,12 @@ final class IfExpressionParser {
private init(components: ArraySlice<String>, tokenParser: TokenParser, token: Token) throws {
var parsedComponents = Set<Int>()
var bracketsBalance = 0
self.tokens = try zip(components.indices, components).compactMap { (index, component) in
#if swift(>=4.1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it would be better to add an extension that defines compactMap on collections for swift 4.1 rather than checking it in place? will be also easier to remove later.

ilyapuchka
ilyapuchka reviewed Oct 1, 2018
View reviewed changes
Copy link
Collaborator

@ilyapuchka ilyapuchka left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Few small comments but otherwise looks good to me.

djbe
djbe requested changes Oct 1, 2018
View reviewed changes
Sources/Variable.swift Outdated
@@ -116,9 +116,32 @@ public struct Variable : Equatable, Resolvable {

return normalize(current)
}

public static func ==(lhs: Variable, rhs: Variable) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, wrap in #if swift.

@djbe
Copy link
Contributor

djbe commented Oct 1, 2018

I agree that we should define compactMap for older swift versions, and maybe we should do this in a _SwiftSupport.swift file, where we can collect all these backwards compat. fixes.

The == can also be implemented there.

@Andrew-Lees11
Copy link
Contributor Author

Hey
Thank you for the quick review. As suggested I have moved all the changes into a single file. This extends Collection to so future compact maps and index should work for Swift 4.0 as well.

djbe
djbe reviewed Oct 1, 2018
View reviewed changes
Sources/_SwiftSupport.swift Outdated
@@ -0,0 +1,42 @@
import Foundation

#if swift(>=4.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I misread that at first and thought you made a typo, until I saw the else clause.
Maybe make this #if !swift(>=4.1)?

@djbe djbe merged commit 4faf8f5 into stencilproject:master Oct 1, 2018
@djbe
Copy link
Contributor

djbe commented Oct 1, 2018

Note for those wondering: the file should be named _SwiftSupport so that it's the first to compile, otherwise Swift doesn't always find the extensions.

@djones6
Copy link

djones6 commented Oct 30, 2018

@djbe Could this be tagged so that SwiftPM can pick up this fix?

@djones6 djones6 mentioned this pull request Oct 30, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilyapuchka ilyapuchka ilyapuchka left review comments

@djbe djbe djbe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Andrew-Lees11 @djbe @djones6 @ilyapuchka