hs-bcrypt
hs-bcrypt copied to clipboard
Newtype for hash values
I spent some resources on finding a mistake I made that would have been caught by the type checker with this change.
I haven't tested this, but would like to hear your opinion before I do: Any objections? Would it be if I started an hspec test suite? Should I factor out an internal module so we can reduce the export list while still allowing people to get access to everything if they really want to?
(Thanks @np!)
I think we should let this one wait a bit and get some more feedback from the community about how they use hs-bcrypt.
On one hand, some (most?) users will probably immediately want to put the result into a library that needs a ByteString (e.g. to save in a file or an SQL database of some form) - with this change, they need to deconstruct the BCryptHash (and reconstruct when they load it back). It is trivial to do, but it is a change from the status quo and some users might find it annoying.
On the other hand, some users will want to pass it around further; this probably applies more for those building more complex applications, and would benefit from the additional type safety. Those applications could define the newtype themselves - it doesn't necessarily have to be provided by the library, but providing it in the library prevents the extra step, and if there are ever libraries that use hs-bcrypt and take a password hash, having a standard newtype would mean they all use the same one.
On your other comments: I think a test suite would be a great idea. I'm not so sure about reducing the import list - everything that is exported is designed to be used by routinely by applications; hashUsesPassword etc... are only needed once you need to 'upgrade' a password to a stronger policy on login, which new applications won't need. Given that it is important to review and upgrade to appropriate hashing policies as available computing power changes, I wouldn't want to discourage people from writing policy upgrade code by placing those exports in an internal module.
I would welcome feedback from other users of hs-bcrypt on whether they would find the newtype useful.
Thanks for the thorough feedback! After reading this I am still in favor of the newtype, but I'll still be a happy user no matter what the outcome of this discussion. (-:
If the decision is for the newtype, I will write some hspec tests.
Not sure about the Internal module any more. I think I was aiming for an abstract newtype, but as you say serialization will require all users to deconstruct that.
Let's hear what others have to say.
it's a good patch and it's something i wanted to do in #10