-
Notifications
You must be signed in to change notification settings - Fork 116
Refactor init 2 #28
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
Refactor init 2 #28
Conversation
9678b53
to
4727ed1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this approach much more than #24 -- looks great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm good with this now! Some nits before we merge, but I think we are on a good trajectory.
/// | ||
/// - note: This is a blocking operation that will run forever, as it's lifecycle is managed by the AWS Lambda Runtime Engine. | ||
@inlinable | ||
public static func run(_ factory: @escaping LambdaHandlerFactory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we use a typealias LambdaHandlerFactory
I would guess we will also offer just a handler closure option. Do we want to name those run() {}
and the factory closures functions like this one createHandlerAndRun() {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally like the shorter name but can consider makeAndRun
in a different PR?
/// | ||
/// - note: This is a blocking operation that will run forever, as it's lifecycle is managed by the AWS Lambda Runtime Engine. | ||
@inlinable | ||
public static func run(_ factory: @escaping (EventLoop) throws -> LambdaHandler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we don't use a typealias
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's because its used in much less place and not user facing, typically you would pass in a ctor here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like the shorter name but can consider wrong commentmakeAndRun
in a different PR?
@@ -113,31 +150,33 @@ public enum Lambda { | |||
private let eventLoop: EventLoop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should move this Lambda.Lifecycle
into another file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do in another PR
motivation: make initialization logic more robust, allowing setup at contructor time and also async bootstrap changes: * break apart "initialization" into two parts: * optional throwing constructor (provider) that takes an EventLoop * optional BootstrappedLambdaHandler protocol that takes an EventLoop and returns async * update core API and logic to support new initialization logic * add tests to various initialization flows
motivation: make initialization logic more robust, allowing setup at contructor time and also async bootstrap changes: * break apart "initialization" into two parts: * optional throwing constructor (provider) that takes an EventLoop * optional BootstrappedLambdaHandler protocol that takes an EventLoop and returns async * update core API and logic to support new initialization logic * add tests to various initialization flows
alternative to #24