Skip to content

Conversation

@csantanapr
Copy link
Member

@csantanapr csantanapr commented Nov 7, 2017

Work in progress
Closes #3
Closes #4

  • create new gradle project
  • use /swift4Action directory instead of /swift3Action
  • update dependencies
  • No external dependencies required
  • Add swift4 tests
  • Add publish steps
  • Update main README
  • Update readme example for Package.swift to use SwiftyRequest
  • Add tests for _Whisk.swift (action invoke, trigger fire)
  • Add CHANGELOG

@csantanapr csantanapr added the wip label Nov 7, 2017
@csantanapr csantanapr changed the title Swift 4 support WIP: Swift 4 support Nov 7, 2017
@jthomas
Copy link
Member

jthomas commented Nov 8, 2017

Looking at the existing packages, it might be worth reviewing these given the changes to Swift 4.

  • SwiftyJSON doesn't seem necessary with Codable support?
  • Is Kitura-net necessary with URLSession?

When the Cloudant Swift lib supports Linux (might be worth including that).

@csantanapr
Copy link
Member Author

csantanapr commented Nov 9, 2017

@jthomas Yeah it will be good to refactor Whisk class to use new stuff from Swift 4

Want to help with that? I created issue #6

In terms of dependencies, if we remove them current users using swift 3.1.1 using source file, would need to refactor their code becasue SwifttyJSON and KituraNet would be removed.

Maybe is not that bad and we just need to doc or blog on how to migrate their code.

@csantanapr
Copy link
Member Author

Regardless if we include the dependency or not we should refactor and remove it if possible in https://github.com/apache/incubator-openwhisk-runtime-swift/blob/master/core/swift3Action/spm-build/_WhiskJSONUtils.swift and https://github.com/apache/incubator-openwhisk-runtime-swift/blob/master/core/swift3Action/spm-build/_WhiskJSONUtils.swift

This way for folks doing zip with pre-compile they are not forced to include the dependencies, because of the epilogue

@csantanapr
Copy link
Member Author

yay epilogue doesn't have any dependencies

@csantanapr
Copy link
Member Author

csantanapr commented Jan 19, 2018

@csantanapr
Copy link
Member Author

Add test for using https://github.com/IBM-Swift/SwiftyRequest as pre-compiled

@csantanapr csantanapr added review and removed wip labels Jan 19, 2018
@csantanapr csantanapr changed the title WIP: Swift 4 support Swift 4 support Jan 19, 2018
@csantanapr
Copy link
Member Author

I reached a stable swift 4 runtime 🎉

@csantanapr
Copy link
Member Author

  • Add tests for _Whisk.swift (action invoke, trigger fire)

@csantanapr
Copy link
Member Author

Added test case for compiling 3rd party package, used SwiftyRequest package, which is very common task to do a http request to API services.

if let json = try? JSONSerialization.jsonObject(with: retval, options: []) as! [String:Any] {
resp = json
} else {
resp = ["error":"Response from server is not a dictionary like"]

Choose a reason for hiding this comment

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

👍 👍

@csantanapr
Copy link
Member Author

Done adding test cases for for both swift 3 and swift 4 for Whisk SDK (invoke, trigger, createTrigger, createRule).

@csantanapr
Copy link
Member Author

added changelog

@csantanapr
Copy link
Member Author

I think I'm done for review and merge.

### Migrating from Swift 3 to Swift 4

### Helper compile.sh helper script
When compiling and packaging your swift 4 now there are a couple of differences
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: couple of differences.


### Helper compile.sh helper script
When compiling and packaging your swift 4 now there are a couple of differences
All your source code needs to be copy to `/swift4Action/spm-build/Sources/Action/` instead of `/swift3Action/spm-build/`
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: needs to be copied

For swift 4 you need specify additional information in Package.swift such as `products` with executable name `Action` and `targets`

You can take a look at the helper script [tools/build/compile.sh](tools/build/compile.sh) to compile and zip your Actions.
Having a project directory `Hello` under a directory `actions` like the following:
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: Have a project

This will produce a zip `build/swift4/Hello.zip`

### SwiftyJSON using single source action file
If you have a swift:3.1.1 action not compile, just as source using the `SwiftyJSON` package, you need to precompile your action and specify the version of SwiftyJSON you wan to use for swift:4 kind action.
Copy link
Contributor

Choose a reason for hiding this comment

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

version of SwiftyJSON you want


### SwiftyJSON using single source action file
If you have a swift:3.1.1 action not compile, just as source using the `SwiftyJSON` package, you need to precompile your action and specify the version of SwiftyJSON you wan to use for swift:4 kind action.
Take into account that tarting with Swift 4 there is better support to manage JSON data natively.
Copy link
Contributor

Choose a reason for hiding this comment

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

Take into account that starting

} else {
return [ "greeting" : "Hello stranger!" ]
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Since some of Swift 3 and Swift 4 tests are the same, should we use a base directory to store common files so we don't keep duplicates? Maybe I'm being too picky?

@paulcastro paulcastro self-assigned this Jan 24, 2018
Copy link
Contributor

@paulcastro paulcastro left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@paulcastro paulcastro merged commit 7acf1d4 into apache:master Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants