Skip to content

Action documentation rewrite and initializer renaming. #325

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 1, 2017
Merged

Action documentation rewrite and initializer renaming. #325

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
322c8c5
Action documentation rewrite and initializer renaming.
andersio Apr 13, 2017
26b2e56
Replace `workProducer` with an explicit `execute` label.
andersio May 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 89 additions & 75 deletions Sources/Action.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,42 +2,45 @@ import Dispatch
import Foundation
import Result

/// Represents an action that will do some work when executed with a value of
/// type `Input`, then return zero or more values of type `Output` and/or fail
/// with an error of type `Error`. If no failure should be possible, NoError can
/// be specified for the `Error` parameter.
/// `Action` represents a repeatable work with varying input and state. Each unit of the
/// repreatable work may output zero or more values, and terminate with or without an
Copy link
Contributor

@mdiep mdiep Apr 16, 2017

Choose a reason for hiding this comment

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

Typo of repeatable here

/// error at some point.
///
/// Actions enforce serial execution. Any attempt to execute an action multiple
/// times concurrently will return an error.
/// The core of `Action` is the `workProducer` closure that is supplied to the
/// initializer. For every execution attempt with a varying input, if the `Action` is
/// enabled, it would invoke `workProducer` with the latest state and the input to obtain
/// a customized unit of work — represented by a `SignalProducer`.
///
/// `Action` enforces serial execution, and disables the `Action` during the execution.
public final class Action<Input, Output, Error: Swift.Error> {
private let deinitToken: Lifetime.Token

private let executeClosure: (_ state: Any, _ input: Input) -> SignalProducer<Output, Error>
private let eventsObserver: Signal<Event<Output, Error>, NoError>.Observer
private let disabledErrorsObserver: Signal<(), NoError>.Observer

/// The lifetime of the Action.
/// The lifetime of the `Action`.
public let lifetime: Lifetime

/// A signal of all events generated from applications of the Action.
/// A signal of all events generated from all units of work of the `Action`.
///
/// In other words, this will send every `Event` from every signal generated
/// by each SignalProducer returned from apply() except `ActionError.disabled`.
/// In other words, this sends every `Event` from every unit of work that the `Action`
/// executes.
public let events: Signal<Event<Output, Error>, NoError>

/// A signal of all values generated from applications of the Action.
/// A signal of all values generated from all units of work of the `Action`.
///
/// In other words, this will send every value from every signal generated
/// by each SignalProducer returned from apply() except `ActionError.disabled`.
/// In other words, this sends every value from every unit of work that the `Action`
/// executes.
public let values: Signal<Output, NoError>

/// A signal of all errors generated from applications of the Action.
/// A signal of all errors generated from all units of work of the `Action`.
///
/// In other words, this will send errors from every signal generated by
/// each SignalProducer returned from apply() except `ActionError.disabled`.
/// In other words, this sends every error from every unit of work that the `Action`
/// executes.
public let errors: Signal<Error, NoError>

/// A signal which is triggered by `ActionError.disabled`.
/// A signal of all failed attempts to start a unit of work of the `Action`.
public let disabledErrors: Signal<(), NoError>

/// A signal of all completed events generated from applications of the action.
Expand All @@ -54,9 +57,12 @@ public final class Action<Input, Output, Error: Swift.Error> {

private let state: MutableProperty<ActionState>

/// Initializes an action that will be conditionally enabled based on the
/// value of `state`. Creates a `SignalProducer` for each input and the
/// current value of `state`.
/// Initializes an `Action` that would be conditionally enabled depending on its
/// state.
///
/// When the `Action` is asked to start the execution with an input value, a unit of
/// work — represented by a `SignalProducer` — would be created by invoking
/// `workProducer` with the latest state and the input value.
///
/// - note: `Action` guarantees that changes to `state` are observed in a
/// thread-safe way. Thus, the value passed to `isEnabled` will
Expand All @@ -68,21 +74,19 @@ public final class Action<Input, Output, Error: Swift.Error> {
/// The various convenience initializers should cover most use cases.
///
/// - parameters:
/// - state: A property that provides the current state of the action
/// whenever `apply()` is called.
/// - enabledIf: A predicate that, given the current value of `state`,
/// returns whether the action should be enabled.
/// - execute: A closure that returns the `SignalProducer` returned by
/// calling `apply(Input)` on the action, optionally using
/// the current value of `state`.
public init<State: PropertyProtocol>(state property: State, enabledIf isEnabled: @escaping (State.Value) -> Bool, _ execute: @escaping (State.Value, Input) -> SignalProducer<Output, Error>) {
/// - state: A property to be the state of the `Action`.
/// - enabledIf: A predicate which determines the availability of the `Action`,
/// given the latest `Action` state.
/// - workProducer: A closure that produces a unit of work, as `SignalProducer`, to
/// be executed by the `Action`.
public init<State: PropertyProtocol>(state property: State, enabledIf isEnabled: @escaping (State.Value) -> Bool, _ workProducer: @escaping (State.Value, Input) -> SignalProducer<Output, Error>) {
deinitToken = Lifetime.Token()
lifetime = Lifetime(deinitToken)

// Retain the `property` for the created `Action`.
lifetime.observeEnded { _ = property }

executeClosure = { state, input in execute(state as! State.Value, input) }
executeClosure = { state, input in workProducer(state as! State.Value, input) }

(events, eventsObserver) = Signal<Event<Output, Error>, NoError>.pipe()
(disabledErrors, disabledErrorsObserver) = Signal<(), NoError>.pipe()
Expand All @@ -106,46 +110,52 @@ public final class Action<Input, Output, Error: Swift.Error> {
self.isExecuting = state.map { $0.isExecuting }.skipRepeats()
}

/// Initializes an action that will be conditionally enabled, and creates a
/// `SignalProducer` for each input.
/// Initializes an `Action` that would be conditionally enabled.
///
/// When the `Action` is asked to start the execution with an input value, a unit of
/// work — represented by a `SignalProducer` — would be created by invoking
/// `workProducer` with the input value.
///
/// - parameters:
/// - enabledIf: Boolean property that shows whether the action is
/// enabled.
/// - execute: A closure that returns the signal producer returned by
/// calling `apply(Input)` on the action.
public convenience init<P: PropertyProtocol>(enabledIf property: P, _ execute: @escaping (Input) -> SignalProducer<Output, Error>) where P.Value == Bool {
/// - enabledIf: A property which determines the availability of the `Action`.
/// - workProducer: A closure that produces a unit of work, as `SignalProducer`, to
/// be executed by the `Action`.
public convenience init<P: PropertyProtocol>(enabledIf property: P, _ workProducer: @escaping (Input) -> SignalProducer<Output, Error>) where P.Value == Bool {
self.init(state: property, enabledIf: { $0 }) { _, input in
execute(input)
workProducer(input)
}
}

/// Initializes an action that will be enabled by default, and creates a
/// SignalProducer for each input.
/// Initializes an `Action` that would always be enabled.
///
/// When the `Action` is asked to start the execution with an input value, a unit of
/// work — represented by a `SignalProducer` — would be created by invoking
/// `workProducer` with the input value.
///
/// - parameters:
/// - execute: A closure that returns the signal producer returned by
/// calling `apply(Input)` on the action.
public convenience init(_ execute: @escaping (Input) -> SignalProducer<Output, Error>) {
self.init(enabledIf: Property(value: true), execute)
/// - workProducer: A closure that produces a unit of work, as `SignalProducer`, to
/// be executed by the `Action`.
public convenience init(_ workProducer: @escaping (Input) -> SignalProducer<Output, Error>) {
self.init(enabledIf: Property(value: true), workProducer)
}

deinit {
eventsObserver.sendCompleted()
disabledErrorsObserver.sendCompleted()
}

/// Creates a SignalProducer that, when started, will execute the action
/// with the given input, then forward the results upon the produced Signal.
/// Create a `SignalProducer` that would attempt to create and start a unit of work of
/// the `Action`. The `SignalProducer` would forward only events generated by the unit
/// of work it created.
///
/// - note: If the action is disabled when the returned SignalProducer is
/// started, the produced signal will send `ActionError.disabled`,
/// and nothing will be sent upon `values` or `errors` for that
/// particular signal.
/// If the execution attempt is failed, the producer would fail with
/// `ActionError.disabled`.
///
/// - parameters:
/// - input: A value that will be passed to the closure creating the signal
/// producer.
/// - input: A value to be used to create the unit of work.
///
/// - returns: A producer that forwards events generated by its started unit of work,
/// or emits `ActionError.disabled` if the execution attempt is failed.
public func apply(_ input: Input) -> SignalProducer<Output, ActionError<Error>> {
return SignalProducer { observer, disposable in
let startingState = self.state.modify { state -> Any? in
Expand Down Expand Up @@ -213,43 +223,47 @@ extension Action: BindingTargetProvider {
}

extension Action where Input == Void {
/// Initializes an action that uses an `Optional` property for its input,
/// and is disabled whenever the input is `nil`. When executed, a `SignalProducer`
/// is created with the current value of the input.
/// Initializes an `Action` that uses a property of optional as its state.
///
/// When the `Action` is asked to start the execution, a unit of work — represented by
/// a `SignalProducer` — would be created by invoking `workProducer` with the latest
/// value of the state.
///
/// If the property holds a `nil`, the `Action` would be disabled until it is not
/// `nil`.
///
/// - parameters:
/// - input: An `Optional` property whose current value is used as input
/// whenever the action is executed. The action is disabled
/// whenever the value is `nil`.
/// - execute: A closure to return a new `SignalProducer` based on the
/// current value of `input`.
public convenience init<P: PropertyProtocol, T>(input: P, _ execute: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T? {
self.init(state: input, enabledIf: { $0 != nil }) { input, _ in
execute(input!)
/// - state: A property of optional to be the state of the `Action`.
/// - workProducer: A closure that produces a unit of work, as `SignalProducer`, to
/// be executed by the `Action`.
public convenience init<P: PropertyProtocol, T>(state: P, _ workProducer: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T? {
self.init(state: state, enabledIf: { $0 != nil }) { state, _ in
workProducer(state!)
}
}

/// Initializes an action that uses a property for its input. When executed,
/// a `SignalProducer` is created with the current value of the input.
/// Initializes an `Action` that uses a property as its state.
///
/// When the `Action` is asked to start the execution, a unit of work — represented by
/// a `SignalProducer` — would be created by invoking `workProducer` with the latest
/// value of the state.
///
/// - parameters:
/// - input: A property whose current value is used as input
/// whenever the action is executed.
/// - execute: A closure to return a new `SignalProducer` based on the
/// current value of `input`.
public convenience init<P: PropertyProtocol, T>(input: P, _ execute: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T {
self.init(input: input.map(Optional.some), execute)
/// - state: A property to be the state of the `Action`.
/// - workProducer: A closure that produces a unit of work, as `SignalProducer`, to
/// be executed by the `Action`.
public convenience init<P: PropertyProtocol, T>(state: P, _ workProducer: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T {
Copy link
Contributor

@mdiep mdiep Apr 18, 2017

Choose a reason for hiding this comment

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

I don't think these names are an improvement over the current names.

  1. state seems less accurate than input. I wouldn't say that this property describes the state of the Action. It's the inputs that are used—consumed in a reactive way.

  2. workProducer is a bit misleading because it's not a producer itself—it's a closure that returns a producer. So I think execute is a preferable name here as well.

Copy link
Member Author

@andersio andersio Apr 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't say that this property describes the state of the Action. It's the inputs that are used—consumed in a reactive way.

The point is though the term input is already bound to the external input from the caller-facing apply() since it is also part of the type name. So calling this input leads to two interpretations of the same term, which do not seem nice for documentation purpose.

After all, this work all derived from a question of "why is it the input when Input is Void".

Moreover, it is just a more specific version of init(state:enabledIf:_:), and the semantic hasn't really changed. If this shouldn't be called state, why should the most generic version be in terms of consistency?

If state is not the right choice, how about external input vs internal input?

workProducer is a bit misleading because it's not a producer itself—it's a closure that returns a producer. So I think execute is a preferable name here as well.

I can't defend workProducer in this regard. But execute doesn't seem right either, since it doesn't really execute but returns something to be executed. workFactory, producerFactory or workCustomizer are possible alternatives.

Copy link
Contributor

@mdiep mdiep May 1, 2017

Choose a reason for hiding this comment

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

I'm 👍 with state.

I can't defend workProducer in this regard. But execute doesn't seem right either, since it doesn't really execute but returns something to be executed. workFactory, producerFactory or workCustomizer are possible alternatives.

True, but I think most things that return SignalProducers are named this way. Look at the methods in Carthage or any of its dependencies, for example.

self.init(state: state.map(Optional.some), workProducer)
}
}

/// The type of error that can occur from Action.apply, where `Error` is the
/// type of error that can be generated by the specific Action instance.
/// `ActionError` represents the error that could be emitted by a unit of work of a
/// certain `Action`.
public enum ActionError<Error: Swift.Error>: Swift.Error {
/// The producer returned from apply() was started while the Action was
/// disabled.
/// The execution attempt was failed, since the `Action` was disabled.
case disabled

/// The producer returned from apply() sent the given error.
/// The unit of work emitted an error.
case producerFailed(Error)
}

Expand Down
12 changes: 12 additions & 0 deletions Sources/Deprecations+Removals.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,18 @@ import Foundation
import enum Result.NoError

// MARK: Obsolete types in ReactiveSwift 2.0.
extension Action where Input == Void {
@available(*, deprecated, renamed:"init(state:_:)")
public convenience init<P: PropertyProtocol, T>(input: P, _ execute: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T? {
self.init(state: input, execute)
}

@available(*, deprecated, renamed:"init(state:_:)")
public convenience init<P: PropertyProtocol, T>(input: P, _ execute: @escaping (T) -> SignalProducer<Output, Error>) where P.Value == T {
self.init(state: input, execute)
}
}

@available(*, unavailable, message: "This protocol has been removed. Constrain `Action` directly instead.")
public protocol ActionProtocol {}

Expand Down
4 changes: 2 additions & 2 deletions Tests/ReactiveSwiftTests/ActionSpec.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ class ActionSpec: QuickSpec {

it("executes the action with the property's current value") {
let input = MutableProperty(0)
let action = Action(input: input, echo)
let action = Action(state: input, echo)

var values: [Int] = []
action.values.observeValues { values.append($0) }
Expand All @@ -318,7 +318,7 @@ class ActionSpec: QuickSpec {

it("is disabled if the property is nil") {
let input = MutableProperty<Int?>(1)
let action = Action(input: input, echo)
let action = Action(state: input, echo)

expect(action.isEnabled.value) == true
input.value = nil
Expand Down