-
Notifications
You must be signed in to change notification settings - Fork 378
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
scratch work of sanitizers and pre/post validation #92
Conversation
As a demonstration, I added a UISwitch to the ViewController. The idea being that the switch is the custom object (non textfield object) to be checked by the user. If it is off, the form doesn't validator.validate() is never called. If it is on, then validation.validate() is called. The validateShouldRun() delegate method being what determines this. |
This is looking really good! I am having some questions about the validateShouldRun()
Sorry I am just kind of thinking out loud.... I think I like the idea of requiring a I guess we could go to 3 hooks:
What do you think? |
ValidatorShouldRun can be a bit confusing. Also, I'm on the same page about shouldValidate returning a Bool. I actually have it do so in this pull request. I also agree with your will/did validate hooks. |
made revisions to validate flow
Frivolous addition of documentation link.
import Foundation | ||
|
||
public protocol Sanitizer { | ||
func sanitize(textField: UITextField) |
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.
Hey. I'm new here so don't want to step on any toes but I have a comment about this. I think the Sanitizer should take a string and output a string instead of taking the text field directly. This would avoid mutating state in addition to following similar api design as the Rule protocol. Additionally this would allow you to remove the sanitizers for loop and use reduce to apply the string transformations.
Additionally, I'm not sure the sanitized string should ever be reapplied back onto the text field. One example of why this isn't a good idea is if I'm trying to validate the textfield as the field is being typed in. If the sanitizer is applied (as the code is currently written) it will be changing the text field as I'm typing which could be a frustrating experience for the user.
Perhaps there should be a distinction between sanitizing text to prepare it for the rules validation process and formatting text for display purposes. In my opinion this library shouldn't be in the business of formatting the text. In that sense applying the sanitization output back on to the text field doesn't make 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.
After thinking about this some more, and looking back at the original ticket for this, I've come to the conclusion that the point of the sanitizers is to format the text. This being the case I don't feel that the sanitizers should run before validation. They should run after validation and they should only be applied on the textfields that have passed validation.
Any sanitization that needs to be done on the strings in textfields in order for rules to run would be avoiding the ultimate purpose of the rules in the first place.. to validate that the contents of the string don't violate whatever constraint that rule is trying to uphold. And if that rule doesn't care about something in the string because it doesn't violate the constraint then it should just ignore it. So why sanitize before the rule?
But this takes be back to my previous comment.. I don't think a text validation library, that markets itself as such, should be in the business of formatting text for display purposes.
Hey @xgalaxy Sorry for the late comment but I agree with your take on text sanitization. It should not be applied pre-validation, if it should ever be applied at all. But to be honest, this pull request is only supposed to be a draft. |
@jpotts18 Took the work from #64 as a basis for sanitizers. Also, started on the flow of validationShouldRun and validationDidRun to see if it's what you were talking about in #82. Pull request is not intended to be merged in it's current state.