Skip to content

Conversation

@fisx
Copy link

@fisx fisx commented Jan 21, 2016

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!)

@A1kmm
Copy link
Owner

A1kmm commented Jan 26, 2016

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.

@fisx
Copy link
Author

fisx commented Jan 26, 2016

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.

@dredozubov
Copy link
Contributor

dredozubov commented Apr 27, 2016

it's a good patch and it's something i wanted to do in #10

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

Successfully merging this pull request may close these issues.

3 participants