Update to new major version of elm-sha1

[TSFoster]

Hey,

I was able to do the work this morning. I've tried to split the commits up into as many logical parts as I can, but 11b90b8 is the main one.

I think it's made about a 20% performance increase, from rough testing.

The changes mean that there were a couple of unnecessary dependencies, and also that the type signatures of toHex and toBase64 changed (for the better, I think). I left those changes till the final commits.

As the changes to toHex and toBase64 mean a major version change anyway, I wanted to ask what you think about renaming toIntList? In the rewrite of elm-sha1, @folkertdev went for toByteValues, which I think is good — it describes what the List Int actually represents, so it gives it better meaning.

[rlopzc]

Man this is awesome!! I see a bunch of improvements, the code is clearer and less deps 👍

Yes, I think it makes more sense to rename that function

Thanks for this

[TSFoster]

Cool, glad it looks good 😄.

I also wondered if it makes sense adding the ability to make digests from Bytes? And if so, only the message, or the key too? If both, then it raises an interesting design question about how/whether to cater for all 4 combinations (String key + String message, Bytes key + String message, …).

[Malax]

Dropping the Result is exactly what I need. I even started to bug some people that wrote other Elm SHA1 packages to drop Result or Maybe so I can open a PR here as well. Great work @TSFoster! :)

[rlopzc]

@TSFoster sorry I've been busy

🤔 tough question. To make simpler this package we could leave it as it is. I mean, if someone has the key in bytes it can convert it to string representation? And the msg too. I don't have a lot of experience using hmac, but what I've used is key and msg as string and not many usages in bytes, being in ruby or JS. I think is not about all the options this package can provide but more about how is going to beused.

What do you think?

[Malax]

I mean, if someone has the key in bytes it can convert it to string representation?

There is something to consider about String vs. Bytes: not every string of bytes is a valid UTF8 string sequence. If this library wants to suit every use-case for hmac-sha1, having a way to use both key and message as raw-bytes seems necessary.

[TSFoster]

As custom types are Very Good and encouraged in Elm, I'd suggest exposing an opaque Key type, which could be made using HmacSha1.Key.from{String,Bytes} or HmacSha1.keyFrom{String,Bytes}.

You could do the same with a Message type, but I think it might be best to just have fromBytes : Key -> Bytes -> Digest and fromString : Key -> String -> Digest.

If you make a Key type, one thing to note is that = on Bytes does not work, so it would maybe be best for a Key to be defined as type Key = Key (List Int) as the non-exposed type currently is.

[rlopzc]

I think HmacSha1.Key.from{String,Bytes} is good. I also agree with fromBytes and fromString, I don't think having a Message module makes sense as we already have the difference in the function type annotation (String and Bytes)

[TSFoster]

Cool. How do you want to move forward with this? I'm happy to make the refactor and add it to this PR/a new one? Or do you want to do it?

In terms of implementation, I'd imagined something like:

module Internal.Key exposing (Key(..))

type Key = Key (List Int)

module HmacSha1.Key exposing (Key, fromString, fromBytes)

import Internal.Key as Key

type alias Key = Key.Key

That's how I've seen handling types that are opaque externally, but not internally

[rlopzc]

I would add the refactor on this PR.

And make just one opaque module HmacSha1.Key

module HmacSha1.Key exposing (Key, fromString, fromBytes)

type Key = Key (List Int)

WDYT?

[TSFoster]

HmacSha1 needs access to the List Int, so HmacSha1.Key would need to (publicly) expose a toByteValues function. By using something like Internal.Key (and not listing Internal.Key as an exposed module in elm.json), the Key data can be shared within the package, but not exposed publicly.

Does that make sense? I don't know how to write it clearly!

[rlopzc]

Ahh I see! Yes, it makes total sense. I need to learn more on package creation! haha. Thanks for the explanation and discussion

[TSFoster]

Cool, no problem. You want to do it, or me?

[rlopzc]

If you have time would be nice, I can do it this weekend if is not done

[TSFoster]

I've added the functionality using the API we discussed. Some work still needs to be done on the tests and the doc tests

[rlopzc]

@TSFoster very nice! I'll finish it up, thank you for this