Skip to content
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

Add a "sponge" reader and writer #10

Closed
wants to merge 5 commits into from
Closed

Add a "sponge" reader and writer #10

wants to merge 5 commits into from

Conversation

cyberdelia
Copy link
Contributor

This is a substandard implementation of #8.
This would close #7, since the base64 encoding is now optional.

for _, k := range r.keys {
msg := verify(nil, r.buf.Bytes(), r.ttl, time.Now(), k)
if msg != nil {
r.buf = bytes.NewBuffer(msg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break?

@kr
Copy link
Contributor

kr commented Sep 2, 2014

I'd much rather leave the existing implementations alone and write the Reader and Writer in terms of those if possible.

b := make([]byte, encodedLen(w.buf.Len()))
n := gen(b, w.buf.Bytes(), iv, time.Now(), w.key)

if _, w.err = io.Copy(w.w, bytes.NewBuffer(b[:n])); w.err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_, w.err = w.w.Write(b[:n])

@kr
Copy link
Contributor

kr commented Sep 2, 2014

Other general thoughts:

  • my instinct would be to put the reader writer interface stuff in another file
  • unit tests?

@kr
Copy link
Contributor

kr commented Sep 2, 2014

Oh, I see, this patch doesn't reuse the existing public functions because it works on the not-base64 binary bytes. How do we feel about having binary-mode as a rider? I'd rather we make a decision on #7 first, then this patch can have a tighter scope.

@cyberdelia
Copy link
Contributor Author

Wow. Well, you have discovered what I meant by substandard. It also lacks basic documentation, the Reset method is useless as is, etc. I'm going to address some of your valuable comments soon.

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.

Does it make sense to support a binary encoding?
2 participants