-
Notifications
You must be signed in to change notification settings - Fork 429
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
Action documentation rewrite and initializer renaming. #325
Conversation
f4a3a92
to
0f2d2b5
Compare
0f2d2b5
to
322c8c5
Compare
Sources/Action.swift
Outdated
/// 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 |
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.
Typo of repeatable here
Sources/Action.swift
Outdated
/// - 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 { |
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 don't think these names are an improvement over the current names.
-
state
seems less accurate thaninput
. I wouldn't say that this property describes the state of theAction
. It's the inputs that are used—consumed in a reactive way. -
workProducer
is a bit misleading because it's not a producer itself—it's a closure that returns a producer. So I thinkexecute
is a preferable name here as well.
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 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 thinkexecute
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.
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'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 SignalProducer
s are named this way. Look at the methods in Carthage or any of its dependencies, for example.
The documentation rewrite in this PR attempts to explain the mechanics around the concept of:
apply()
producer attempting to create and start a unit of work.Consequently,
Action(input:_:)
has been renamed toAction(state:_:)
, since input refers to the varying input associated with execution attempts.