Skip to content
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

Throwing initializer #4

Closed
radex opened this issue Feb 27, 2016 · 10 comments
Closed

Throwing initializer #4

radex opened this issue Feb 27, 2016 · 10 comments

Comments

@radex
Copy link

radex commented Feb 27, 2016

A good suggestion from https://twitter.com/DeFrenZ/status/703521308550762496:

Instead of having the Validated initializer return nil if the constraint isn't met, you could throw a generic error. If you don't care about the error, you could still ignore it using try?, but you could also easily log it to the console when it's not expected (but not fatal). And you'd have an error like "A value of Foo didn't pass BarValidator" without having to type it yourself.

@cdzombak
Copy link

Given that these validations are constraints important enough to be elevated into the type system, my gut says using throws to handle failures feels right.

And as @radex notes, if you still want to use failable-initializer style, try? is short enough.

@Ben-G
Copy link
Owner

Ben-G commented Feb 28, 2016

This sounds like a good idea to me. I like that we can preserve the easy case with a try? while also being able to leverage Swift's error handling system.

I just prototyped this in a playground and came up with the following ErrorType:

struct ValidatorError: ErrorType, CustomStringConvertible {
    let wrapperValue: Any
    let validator: Any.Type

    var description: String {
        return "Value: \(wrapperValue) <\(wrapperValue.dynamicType)>, failed validation of Validator: \(validator.self)"
    }
}

And this initializer implementation:

    public init(_ value: WrapperType) throws {
        guard V.validate(value) else {
            throw ValidatorError(
                wrapperValue: value,
                validator: V.self)
        }

        self.value = value
    }

For my example this will result in the following default error message:

Value: ASA <String>, failed validation of Validator: And<AllCapsLatinStringValidator, LongerThan10Chars>

I'm testing this together with the logical operators from #3. I'm providing the wrapper value and the Validator metatype as part of the error to enable more complex error handling (and potentially overriding description with a localized default).

@radex @cdzombak Feedback appreciated 😃

@tomquist
Copy link
Contributor

It would be cool to know exactly which validators failed when using aggregation validators like And/Or/Not. Therefore validators should also be able to throw errors to pass validation failures up to the root of the validation tree. Unfortunately this results in more complex validators. However, using protocol extensions, we are able to make throwing validators optional:

struct ValidatorError: ErrorType, CustomStringConvertible {
    let wrapperValue: Any
    let validator: Any.Type
    let errors: [ErrorType]

    var description: String {
        return "Value: \(wrapperValue) <\(wrapperValue.dynamicType)>, failed validation of Validator: \(validator), errors: \(errors)"
    }
}
public protocol Validator {
    typealias WrappedType
    static func validate(value: WrappedType) -> Bool
    static func validate(value: WrappedType) throws
}
extension Validator {
    public static func validate(value: WrappedType) throws {
        guard validate(value) else {
            throw ValidatorError(wrapperValue: value, validator: self, errors: [])
        }
    }
}

Now all leaf validators continue implementing static func validate(value: WrappedType) -> Bool and aggregation validators additionally implement static func validate(value: WrappedType) throws:

extension And {
    public static func validated(value: V1.WrappedType) throws {
        let _: Void = try V1.validate(value)
        let _: Void = try V2.validate(value)
    }
}
extension Or {
    public static func validated(value: V1.WrappedType) throws {
        do { let _: Void = try V1.validate(value) } catch let error1 {
            do { let _: Void = try V2.validate(value) } catch let error2 {
                throw ValidatorError(wrapperValue: value, validator: self, errors: [error1, error2])
            }
        }
    }
}
extension Not {
    public static func validated(value: V1.WrappedType) throws {
        do { let _:Void = try V1.validate(value) } catch { return }
        throw ValidatorError(wrapperValue: value, validator: self, errors: [])
    }
}

@Ben-G
Copy link
Owner

Ben-G commented Feb 28, 2016

I'm a little bit content about adding throwing validate methods (though I had prototyped this variation as well, initially).

I think this library should be mostly utilized in places where we are fairly certain that a validation will pass. I thought of the validation step more of a confirmation of an already inherent type rather than a validation that will likely fail.

With this use case in mind I think we will almost never be able to recover from an error in any useful manner.

By allowing to throw arbitrary errors from the validate function we open the door to a lot of control-flow complexity based on the type validation, which I think is not the responsibility of this library.

I personally would handle complex validation outside of Validated then use the library to express the constraints we have manifested in code within the type system.

A very abstract of a signup process would look like this:

// Example: Signup Process:
UserInput(String) -> complexBusinessLogic -> Validated<User, NewUserValidator>

Validation of individual Strings (to match PW requirements, email address requirements, etc.) would happen outside of Validated; after the signup process we receive a valid User that is guaranteed to be a NewUser (not an existing one).

TL;DR; I think of Validated more as a type confirmation library and wouldn't use it, i.e.for validating user input directly.


P.S.: While writing this, I realized that I used a lot of string validation examples in my tests and the docs. Validated could certainly be used this way (including complex control flow for failed validations), currently however I think it steps outside of the boundaries what this tiny library should responsible for.

But I might be entirely wrong, so I'm looking forward to seeing where a discussion takes us 😃


P.P.S: It seems like we currently have three different options to implement this:

1.Error for Any Type of Validation Failure - as suggested by my first comment)
2.Error that lists the individual ErrorTypes thrown by all validators
3. Error that Specifies which Validation Failed, but doesn't allow the Validations to throw custom errors (they can only return a Bool) - a compromise?

@Ben-G
Copy link
Owner

Ben-G commented Feb 28, 2016

@tomquist There's also some additional complexity that I believed is unaddressed right now? Depending on the type of composite validator, the validator might pass even if one of the validators throws an error (e.g. using OR). Success or failure of the logical composition would have to be handled separately?

@tomquist
Copy link
Contributor

My initial intention when designing my proposal was not to be able to use validators for user input validation and automated recovery (which is also an interesting use case probably better handeled by a dedicated framework). I thought of it to be more like a debugging tool in case validators fail when you don't expect them to. But you are right, for this simple use case the proposal may be somewhat over-engineered.

Here are my comments to the three options:

1.Error for Any Type of Validation Failure - as suggested by my first comment)

This may fit best to the initial intention of this library to be a tiny type confirmation library

2.Error that lists the individual ErrorTypes thrown by all validators

I don't think this is overly useful. E.g. when using the OR validator, some validators may fail but the logical condition still succeeds or when using NOT you are not interested in the error produced by the validator

3.Error that Specifies which Validation Failed, but doesn't allow the Validations to throw custom errors (they can only return a Bool) - a compromise?

This is pretty much what I tried to implement. However, I had to move the static func validate(value: WrappedType) throws within the protocol to be able to implement custom behavior in compositional validators. There may be a better solution where we can hide the throwing validate method from the protocol.

@cdzombak
Copy link

I may suggest changing

return "Value: \(wrapperValue) <\(wrapperValue.dynamicType)>, failed validation of Validator: \(validator.self)"

to

return "Value '\(wrapperValue)' <\(wrapperValue.dynamicType)> failed validation by Validator: \(validator.self)"

which results in messages like

Value 'ASA' <String> failed validation by Validator: And<AllCapsLatinStringValidator, LongerThan10Chars>

Moving on, my 2¢:

Broadly, and admittedly without experimenting for more than a few minutes, @tomquist's proposal is interesting but feels too complex for a tiny pseudo-dependent-types library.

  • It adds significant complexity (∴ room for implementation errors) to the aggregation validators.
  • And it looks like it duplicates the same method in the Validator protocol—though we do provide a default implementation of the throwing version, it still feels messy to me.

I thought of it to be more like a debugging tool in case validators fail when you don't expect them to.

I can see the value here in theory, even when we don't expect validation errors to be recoverable.

But overall I agree with the "1.Error for Any Type of Validation Failure" goal, unless we can achieve "Error that Specifies which Validation Failed, but doesn't allow the Validations to throw custom errors (they can only return a Bool)" without adding public complexity. I really would like to keep Validators responsible for only returning a Bool.

Hope that makes sense.

edit: and, it's unclear to me precisely how let errors: [ErrorType] is supposed to be populated or consumed.

@radex
Copy link
Author

radex commented Feb 29, 2016

I think this library should be mostly utilized in places where we are fairly certain that a validation will pass. I thought of the validation step more of a confirmation of an already inherent type rather than a validation that will likely fail.

That makes sense to me. From my perspective, what I care about is not really having an error caught so I can deal with it. Rather, I just want the failed conversions logged to the console and otherwise treat them as failed assertions.


Aside:

In my apps, I have a helper called assertLog, which is essentially a variant of assertionFailure that:

  • in development, crashes immediately so I can catch a bug quickly
  • in production, the message is only logged to the console, for diagnostics purposes
  • also, in production, the message is logged remotely via Crashlytics so I'm alerted of it

What I would like is to be able to convert from T to Validated<T, V>, where a failed conversion does the above steps automagically. I think I should be able to extend Validated and add a custom initializer that does this, though.

@Ben-G
Copy link
Owner

Ben-G commented Mar 1, 2016

Thanks a lot for your input! I've added a PR based on our discussion: #7

I've found a way to keep the failable initializer around, which I'm pretty happy with. Let me know what you think!

@Ben-G
Copy link
Owner

Ben-G commented Mar 3, 2016

Closed by #7

@Ben-G Ben-G closed this as completed Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants