-
Notifications
You must be signed in to change notification settings - Fork 45
RFC: Nesting #23
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
RFC: Nesting #23
Conversation
a271b50
to
943b33f
Compare
motivation: in some cases users may want access to the shutdown errors changes: add optoinal callback to shutdown providing access to shutdown errors
motivation: add ability to create a hirearchy of lifecycles to help manage more complex systems changes: * define top-level lifcycle where signal handling and backtraces are handled * conform Lifecycle to LifecycleItem so that lifecycles can be nested * change shutdown to return an error to better conform with LifecycleItem api
import Dispatch | ||
import Logging | ||
|
||
public struct TopLevelLifecycle { |
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.
In #14 (comment) the names ServiceLifecycle
and ComponentLifecycle
surfaced -- I think those are good names 👍 How about using those.
Would the module become swift-service-lifecycle?
public struct TopLevelLifecycle { | ||
private let lifecycle = Lifecycle(label: "\(TopLevelLifecycle.self)") | ||
|
||
/// Starts the provided `LifecycleItem` array and waits (blocking) until a shutdown `Signal` is captured or `Lifecycle.shutdown` is called on another thread. |
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.
minor: Item has been nagging me a bit... are those not more like "tasks" to be or "phases"? Item does not feel like it is "part of" Lifecycle.
LifecycleTask
would be my preference but could survive with existing, if people like it strongly
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 like task
/// - parameters: | ||
/// - configuration: Defines lifecycle `Configuration` | ||
/// - callback: The handler which is called after the start operation completes. The parameter will be `nil` on success and contain the `Error` otherwise. | ||
public func start(configuration: Configuration = .init(), callback: @escaping (Error?) -> Void) { |
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.
public func start(configuration: Configuration = .init(), callback: @escaping (Error?) -> Void) { | |
public func start(configuration: Configuration = .init(), _ callback: @escaping (Error?) -> Void) { |
in alignment with #26
/// | ||
/// - parameters: | ||
/// - callback: The handler which is called after the start operation completes. The parameter will be `nil` on success and contain the `Error` otherwise. | ||
public func shutdown(callback: @escaping (Error?) -> Void = { _ in }) { |
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.
public func shutdown(callback: @escaping (Error?) -> Void = { _ in }) { | |
public func shutdown(_ callback: @escaping (Error?) -> Void = { _ in }) { |
items2.forEach { XCTAssertEqual($0.state, .shutdown, "expected item to be shutdown, but \($0.state)") } | ||
items3.forEach { XCTAssertEqual($0.state, .shutdown, "expected item to be shutdown, but \($0.state)") } | ||
} | ||
} |
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.
Looks good to me in general.
Naming wise ServiceLifecycle and ComponentLifecycle sound good.
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.
A component having more components could be a thing but not strictly necessary...
/// | ||
/// - parameters: | ||
/// - items: one or more `LifecycleItem`. | ||
func register(_ items: [LifecycleItem]) { |
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.
don't these need to be public func
? The tests seem to all use @testable import
which might hide this?
replaced by #34 |
proposal for #14, do not merge
the names are terrible, but lets agree on the concept, then find great names