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

adder: add support for specifying the hash function #3919

Merged
merged 2 commits into from
May 14, 2017

Conversation

kevina
Copy link
Contributor

@kevina kevina commented May 12, 2017

Closes #3917

License: MIT
Signed-off-by: Kevin Atkinson [email protected]

@kevina kevina added the status/in-progress In progress label May 12, 2017
@kevina
Copy link
Contributor Author

kevina commented May 12, 2017

@whyrusleeping this p.r. will unconditionally set the Cid version to 1 if the --hash option is used, even if the value is sha2-256.

@whyrusleeping
Copy link
Member

Great! I would just like to see a test that adds some directory stuff recursively too

@kevina
Copy link
Contributor Author

kevina commented May 13, 2017

I'm working on it. Raw leaves are a bit of a problem. They need proper support for being used with a hash function other then the default sha2-256.

@kevina kevina force-pushed the kevina/add-hash-fun branch 2 times, most recently from cdc1c83 to 322a1f5 Compare May 13, 2017 05:20
@kevina
Copy link
Contributor Author

kevina commented May 13, 2017

@whyrusleeping okay this should be good to go, the first commit "Rename NewBlock to NewBlockV0" is not necessary, I did it as a debugging aid and it might be useful to keep. If not it can be cleanly dropped.

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Great! This look good to me 👍 I would like the NewBlockV0 change reverted though, its just a bit too much noise in the PR for my tastes right now

@whyrusleeping
Copy link
Member

Then theres a code climate issue, documenting the NewNodeWithPrefix would be very helpful

@kevina
Copy link
Contributor Author

kevina commented May 14, 2017

@whyrusleeping both issues should be addressed now

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @kevina :)

@whyrusleeping whyrusleeping merged commit a4ffefd into master May 14, 2017
@whyrusleeping whyrusleeping deleted the kevina/add-hash-fun branch May 14, 2017 22:27
@whyrusleeping whyrusleeping removed the status/in-progress In progress label May 14, 2017
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.

2 participants