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 Codable Support for Swift 4.x #23

Merged

Conversation

csantanapr
Copy link
Member

@csantanapr csantanapr commented Feb 14, 2018

Codable Support with Swift 4.x

Some examples of using Codable In and Out

Codable style function signature

Create file helloCodableAsync.swift

// Domain model/entity
struct Employee: Codable {
  let id: Int?
  let name: String?
}
// codable main function
func main(input: Employee, respondWith: (Employee?, Error?) -> Void) -> Void {
    // For simplicity, just passing same Employee instance forward
    respondWith(input, nil)
}
wsk action update helloCodableAsync helloCodableAsync.swift swift:4.1

ok: updated action helloCodableAsync

wsk action invoke helloCodableAsync -r -p id 42 -p name Carlos
{
    "id": 42,
    "name": "Carlos"
}

Codable Error Handling

Create file helloCodableAsync.swift

struct Employee: Codable {
    let id: Int?
    let name: String?
}
enum VendingMachineError: Error {
    case invalidSelection
    case insufficientFunds(coinsNeeded: Int)
    case outOfStock
}
func main(input: Employee, respondWith: (Employee?, Error?) -> Void) -> Void {
    // Return real error
    do{
        throw VendingMachineError.insufficientFunds(coinsNeeded: 5)
    } catch {
        respondWith(nil, error)
    } 
}
wsk action update helloCodableError helloCodableError.swift swift:4.1

ok: updated action helloCodableError

wsk action invoke helloCodableError -b -p id 42 -p name Carlos
{
"name": "helloCodableError",
"response": {
  "result": {
    "error": "insufficientFunds(5)"
  },
"status": "application error",
"success": false
}

@csantanapr
Copy link
Member Author

Depends on #22
Closes #2

@csantanapr
Copy link
Member Author

csantanapr commented Feb 14, 2018

TODOs

  • Handle when input params is empty
  • Add tests cases for Codable

@csantanapr csantanapr added the wip label Feb 14, 2018
@csantanapr csantanapr force-pushed the swift4_2_swift40+swift41+codable branch from 0431808 to 8d3adc3 Compare February 14, 2018 17:53
@jthomas
Copy link
Member

jthomas commented Mar 2, 2018

I've been testing out Codable support from the sample above in my library for wrapping Swift actions to produce binaries.
https://github.com/jthomas/OpenWhiskAction/blob/swift4/Sources/epilogue.swift

It's all working great but I made a few changes to improve the developer experience that we'd probably want to include. Here are the things I found....

  • Support @escaping closures to allow users to use with async calls.
func _run_main<In: Codable, Out: Codable>(mainFunction: (In, @escaping (Out?, Error?) -> Void) -> Void) {
}

This just needs the annotation adding and doesn't affect existing code or people who don't use that.

This helps the user understand when there has been problems using the Codable API versus something else.

  • Sync method style APIs should probably support throws. This enables uses to use normal error handling to surface issues.

I'm away on vacation for a week but will attempt to push a PR when I return.

@csantanapr
Copy link
Member Author

csantanapr commented Mar 2, 2018

Thanks @jthomas for the review and starting the work to externalize the epilogue in to it's own library/package.

Support @escaping closures to allow users to use with async calls.

func _run_main<In: Codable, Out: Codable>(mainFunction: (In, @escaping (Out?, Error?) -> Void) -> Void) {
}

This just needs the annotation adding and doesn't affect existing code or people who don't use that.

Guess what I have in my TODOs?

TODOS:

  • Finish with the Codable test cases
  • Use @escaping instead for the callback
  • Refresh again with latest 4.1 snapshot
  • With now 4.2 announced 4.1 is stable, so skip/remove 4.0 support only do 4.1 for now

https://github.com/jthomas/OpenWhiskAction/blob/swift4/Sources/epilogue.swift#L84-L93
This helps the user understand when there has been problems using the Codable API versus something else.

I will add the one for more verbose errors 👍

  • Improved error handled to give more descriptive messages, e.g.

Sync method style APIs should probably support throws. This enables uses to use normal error handling to surface issues.

I thought about this one, will look into this but probably a different PR since it's an improvement to dictionary sync that we have today.
#29

Super happy that someone is trying this stuff 🎉

@csantanapr
Copy link
Member Author

csantanapr commented Mar 2, 2018

Woot !

  • Finish with the Codable test cases

@csantanapr
Copy link
Member Author

csantanapr commented Mar 2, 2018

Woot !

  • Refresh again with latest 4.1 snapshot

@csantanapr
Copy link
Member Author

csantanapr commented Mar 2, 2018

Woot !

  • Use @escaping instead for the callback

@csantanapr
Copy link
Member Author

csantanapr commented Mar 3, 2018

Woot !

  • Improved error handled to give more descriptive messages, e.g.

@csantanapr csantanapr added enhancement and removed wip labels Mar 3, 2018
@csantanapr
Copy link
Member Author

With now 4.2 announced 4.1 is stable, so skip/remove 4.0 support only do 4.1 for now

Going to move this to a new issue #30

@csantanapr
Copy link
Member Author

I think I'm ready for review and final merge of this PR

@csantanapr csantanapr force-pushed the swift4_2_swift40+swift41+codable branch from ed3f602 to 2b04cde Compare March 3, 2018 03:55
@csantanapr csantanapr force-pushed the swift4_2_swift40+swift41+codable branch from 2b04cde to c29147e Compare March 3, 2018 03:58
"success": false
}
```

### Packaging an action as a Swift executable using Swift 4
Copy link
Contributor

@paulcastro paulcastro Mar 7, 2018

Choose a reason for hiding this comment

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

Hmmmm, can't edit each line individually. Here are some typos:

Line 209: All your source code needs to be copied ...
Line 231: Take into account that starting ...

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the readme with typos fixes @paulcastro

_whisk_print_error(message: "Error serializing data to JSON, data conversion returns nil string", error: nil)
}
} catch {
_whisk_print_error(message: "JFailed to encode Dictionary type to JSON string:", error: error)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's JFailed ? Typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah Typo

@paulcastro paulcastro merged commit 3ccad3b into apache:master Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants