Skip to content

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Apr 24, 2020

motivation: better documentation, happier users

changes: add more inforamtion on public APIs

motivation: better documentation, happier users

changes: add more inforamtion on public APIs
@tomerd tomerd requested review from fabianfett and yim-lee April 24, 2020 03:37

/// A processing closure for a Lambda that takes a `In` and returns a `Result<Out, Error>` via a `CompletionHandler` asynchronously.
/// An asynchronous Lambda Closure that takes a `In: Decodable` and returns a `Result<Out: Encodable, Error>` via a completion handler.
public typealias CodableLambdaClosure<In: Decodable, Out: Encodable> = (Lambda.Context, In, @escaping (Result<Out, Error>) -> Void) -> Void
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to move the typealias up right before the matching Lambda.run function? For readability I prefer if typealiases to be defined as close to the usage as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Looks really good! Some small nits.

///
/// - note: This is a blocking operation that will run forever, as its lifecycle is managed by the AWS Lambda Runtime Engine.
public static func run(_ factory: @escaping LambdaHandlerFactory) {
public static func run(_ factory: @escaping ByteBufferLambdaHandlerFactory) {
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 not sure if I like this renaming. Sure, the Handler that we want to see returned is a ByteBufferLambdaHandler and all other protocols inherit from it. But the new naming gives it IMHO an exclusive feel, especially since we don't have an EventLoopLambdaHandlerFactory or a LambdaHandlerFactory.

This is not a strong con, just wanted to point it the implication it gave me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/// `ByteBufferLambdaHandler` factory.
///
/// A function that takes a `EventLoop` and returns an `EventLoopFuture` of a `ByteBufferLambdaHandler`
public typealias ByteBufferLambdaHandlerFactory = (EventLoop) -> EventLoopFuture<ByteBufferLambdaHandler>
Copy link
Member

Choose a reason for hiding this comment

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

As stated above: Do we maybe want to move this typealias up? Maybe even rename:

Lambda.HandlerFactory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomerd
Copy link
Contributor Author

tomerd commented Apr 28, 2020

@fabianfett @yim-lee feedback addressed

Copy link
Contributor

@yim-lee yim-lee left a comment

Choose a reason for hiding this comment

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

👍

@tomerd tomerd merged commit 2324074 into master Apr 28, 2020
@tomerd tomerd deleted the fix/api-docs branch April 28, 2020 21:31
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