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

adding tests for blake3 and fixing an error handling bug. #158

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

laudiacay
Copy link
Contributor

@laudiacay laudiacay commented Jun 30, 2022

Re-exporting blake3 constants in the right places so we can support it elsewhere (like go-verifcid, so that I can add support for approving fixed-length blake3 as a "good hash" there 😄 ) Draft because there are no tests yet.

edit: it doesn't exactly do this anymore, check comments below...

@laudiacay laudiacay marked this pull request as ready for review June 30, 2022 23:52
@laudiacay
Copy link
Contributor Author

tests passed. I also found a bug in what was being displayed for ErrInconsistentLen and fixed the message to make a little more sense, and added a few comments for clarity... the way the logic was before, it was just gonna print like the same number twice in the "expected blah got bleh" message no matter what.

@laudiacay
Copy link
Contributor Author

@Kubuxu @aschmahmann ready 2 go when u r :)

core/registry.go Outdated Show resolved Hide resolved
multihash.go Outdated Show resolved Hide resolved
multihash.go Outdated Show resolved Hide resolved
multihash.go Outdated Show resolved Hide resolved
multihash.go Outdated Show resolved Hide resolved
@Jorropo
Copy link
Contributor

Jorropo commented Jul 1, 2022

The ErrInconsistentLen change looks good to me thx, but I don't think we need the rest.

@laudiacay laudiacay changed the title Adding blake3 support adding tests for blake3 and fixing an error handling bug. Jul 1, 2022
@laudiacay
Copy link
Contributor Author

ok i changed the title to fit. i'm gonna leave the new blake3 tests too. committing now.

@Jorropo
Copy link
Contributor

Jorropo commented Jul 1, 2022

@laudiacay do the tests passes ?

I think you need to register blake3 in the test suite else they wont find the hasher. Simply add this line to your imports (in the tests) _ "github.com/multiformats/go-multihash/register/blake3", naming an import _ (the blank indentifier) will run of it's init functions without adding it's exports to the file scope.

@laudiacay
Copy link
Contributor Author

register/all gets blake3 and the tests get register/all!

@Jorropo
Copy link
Contributor

Jorropo commented Jul 1, 2022

Ok good mb

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

SGTM (need green CI first) (I don't have merge powers here)

@marten-seemann
Copy link
Contributor

Re-exporting blake3 constants in the right places so we can support it elsewhere

I don't see that anywhere. As far as I can tell, this PR is 1. changing the error messages and 2. adding blake3 tests. Am I missing something?

@Jorropo
Copy link
Contributor

Jorropo commented Jul 1, 2022

I don't see that anywhere.

@marten-seemann

It used to do it but I've asked for it to be removed, I think people that want blake3 can just import _ "github.com/multiformats/go-multihash/register/blake3"

@laudiacay
Copy link
Contributor Author

@marten-seemann it's not doing that anymore- it's fixing an error message and adding tests. I was doing that before but then @Jorropo let me know that that was unnecessary :P didn't quite understand go imports with side effects until just now

@laudiacay
Copy link
Contributor Author

are we good to merge?

@aschmahmann
Copy link
Contributor

@laudiacay is this PR just:

  1. Change the error message to give more info
  2. Rename a variable to be clearer
  3. Add some blake3 tests

If so LGTM, although if you could squash the commits together with a clear commit message that'd be great.

@laudiacay
Copy link
Contributor Author

Yep done

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -27,11 +27,12 @@ var (

// ErrInconsistentLen is returned when a decoded multihash has an inconsistent length
type ErrInconsistentLen struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Notes for a future PR, IMO, this error should wrap io.UnexpectedEOF.

@aschmahmann aschmahmann merged commit f62cf07 into multiformats:master Jul 26, 2022
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
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.

4 participants