Skip to content

Conversation

@achingbrain
Copy link
Member

This appears to have been missed off, unless it's been done on purpose?

Makes MultihashHasher consistent with MultibaseEncoder, BlockEncoder, etc.

Also fixes a small typo.

This appears to have been missed off, unless it's been done on purpose.

Makes `MultihashHasher` consistent with `MultibaseEncoder`, `BlockEncoder`,
etc.
@achingbrain achingbrain requested review from Gozala and rvagg June 23, 2021 14:23
@rvagg
Copy link
Member

rvagg commented Jun 24, 2021

I'm not sure about the reason for the omission, code might be even more useful here. I'm suspecting a bias toward minimalism to give maximum composability. @Gozala will have to answer to that though.

This is fine by me unless @Gozala registers an objection.

@Gozala
Copy link
Contributor

Gozala commented Jun 24, 2021

I'm not sure about the reason for the omission, code might be even more useful here. I'm suspecting a bias toward minimalism to give maximum composability. @Gozala will have to answer to that though.

fewer requirements on interface is easier to satisfy, so unless there’s a compelling reason (which didn’t exist in multiformats at a time) it’s best to keep things out until necessary. And if it’s necessary in 20% of cases maybe separate interface for those is a better option still

@achingbrain
Copy link
Member Author

achingbrain commented Jun 24, 2021

unless there’s a compelling reason

This will be useful to list configured hash algorithms in a way that is easily digestible for a human without consumers having to maintain their own lookup tables.

For example js-ipfs does this to list and validate which hash algorithms are supported on the cli and follows the same pattern to validate incoming http args and needs this PR in order to ultimately remove the dependencies on multihash*, multicodec, and multibase and use multiformats instead.

code might be even more useful here

Good point, this should also be in the interface, as it is with MultibaseEncoder and BlockEncoder. I've added this.

@achingbrain achingbrain changed the title feat: add name field to MultihashHasher interface feat: add name and code fields to MultihashHasher interface Jun 24, 2021
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

As it so happens, these last couple of days I've run into precisely this problem with some HAMT code. We record the multihash code for the algorithm used to hash entries in the root of the structure and I'm typing the interfaces now and it accepts a MultihashHasher as your hash algorithm ... but I can't get code off the interface!
So I'm fine with this, I think it's reasonable and I see multiple usecases for it.

@rvagg rvagg merged commit 31f94f7 into master Jun 25, 2021
@rvagg rvagg deleted the feat/add-hasher-name-to-interface branch June 25, 2021 03:37
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