-
Notifications
You must be signed in to change notification settings - Fork 16
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
Some additions / modifications to password validation #27
Conversation
cc @HirotoShioi in case he wants to do a review of this 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 think this looks really good!
I left a couple comments, but I don't think any of these should necessarily block it being merged in!
I think this should be most of the code in the module. I've also started on a more clear explanation in how to use this module, but that will need a bit more iterations to have it be readable and usable for new users of this module. |
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.
This looks good, thanks for fixing it up!
…dded Ord instance and documentation to 'ValidPasswordPolicy
Ok, I've finished adding documentation and checking how the Haddocks look. Do tell if anything obvious stands out, or if something is confusing. |
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 took another look at this and I think it looks pretty good!
Thanks again for fixing this up and getting it ready to merge in.
Also, please feel free to credit yourself in the ChangeLog.md
, since you did a bunch of the work for this. (Although I guess thinking about this, I haven't been crediting myself in the ChangeLog, so I guess it makes sense for neither of us to credit ourselves!)
TODO:
|
…pile time, with a caveat (can't set 'charSetPredicate')
…ll tests to guarantee consistency
Ok, added TemplateHaskell function and an extra |
Ok, I think I'm done with all the additions and documentation. |
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 took a look a this and it looks really good to me!
I went through the
validate-password
code on my own laptop at some point and checked the comments, tried to make it a bit more DRY and tried to separate the policy validation from the password validation.I'll probably do a last pass over the entire module for correctness and reader-friendliness just before we'll merge everything in.