-
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
Add convenience inits to Action to use a ValidatingProperty as its state. #637
Add convenience inits to Action to use a ValidatingProperty as its state. #637
Conversation
@@ -411,6 +411,21 @@ class ActionSpec: QuickSpec { | |||
|
|||
expect(values) == [3, 4, -11] | |||
} | |||
it("is disabled if the validating property does not hold a valid value") { |
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.
There should be an empty line before the it
here.
typealias PropertyType = ValidatingProperty<Int, TestValidationError> | ||
let decisions: [PropertyType.Decision] = [.valid, .invalid(.generic), .coerced(10, nil)] | ||
let input = PropertyType(0, { decisions[$0] }) | ||
let action = Action(validating: input, execute: echo) |
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.
Can you also test the other variant of this method?
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 simply copied the one from unwrapping
and adjusted it to work with the ValidatingProperty
. Is that ok?
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 works. 👍
Sources/Action.swift
Outdated
/// - state: A `ValidatingProperty` to be the state of the `Action`. | ||
/// - execute: A closure that produces a unit of work, as `SignalProducer`, to | ||
/// be executed by the `Action`. | ||
public convenience init<T, E>(validating state: ValidatingProperty<T, E>, execute: @escaping (T, Input) -> SignalProducer<Output, Error>) { |
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.
The Action
is not validating the state though. It is accepting validated results. Perhaps validated
as the label instead?
/// - state: A `ValidatingProperty` to be the state of the `Action`. | ||
/// - execute: A closure that produces a unit of work, as `SignalProducer`, to | ||
/// be executed by the `Action`. | ||
public convenience init<T, E>(validated state: ValidatingProperty<T, E>, execute: @escaping (T, Input) -> SignalProducer<Output, Error>) { |
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.
The label validated:
doesn't quite read right to me. What do you all think about validatedState:
instead? I think if we're not going to try and make it read as a phrase, then using a noun to label the parameter makes sense.
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.
Why is it not right though? Given an email address for example, Action(validated: email)
looks fine for me. That’s said if we have to be more explicit, it could be validValuesFrom:
or predicating
if a gerund is preferred.
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.
(Ok time to see how much of my high school linguistics I can remember!) The question in my mind is still "validated what?" I think it's leaving subject implicit: yes, the email is validated, but we already knew that from its type — saying so with the label feels redundant to me. Saying validated state makes it clearer that the email (which is validated) is playing the role of the action's state.
For me this seems to fall under the Include all the words needed to avoid ambiguity principle in the API Design Guidelines. Specifically, it seems like a subject/object ambiguity.
(As an aside, I already apparently violated a guideline in the first place by using unwrapping:
to form a phrase using the base name of an initialiser.)
One further idea: in practice we're still unwrapping the validated state values with this overload. How about the label unwrappingValidated:
? I'm not really too attached to my reasoning above given we're already in a grammatical grey-area, and I like how it connects this new initialiser to the very similar unwrapping:
initialiser. Thoughts?
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.
@ReactiveCocoa/reactiveswift @mdiep @Marcocanc Any second opinion?
Motivation
I think
ValidatingProperty
andAction
play nicely together.Example: There's an email input field whose values are sent to a
ValidatingProperty
that uses an email validator. A "submit" action can now take the validated property as its state. The action'sisEnabled
property can easily be bound to the submit button'sisEnabed
binding target.Changes
This PR contains convenience initializers to use a validating property as an Action's state.
Checklist