Open warpfork opened 3 years ago
I think it's probably reasonable to switch over to using go-multicodec already. Having something auto-generated is very nice.
On the other hand, it's another repo and another line item in go.mod
.
If we do switch over to using go-multicodec, a few subquestions would remain:
type Code uint64
typedef. Should we start using that type name everywhere instead of bare uint64
?go-multicodec
repo name might itself be confusing -- if we ever want to address that, best to do it immediately before spreading more usage of it.would strongly suggest not having #136 block on this. I think we should spend our time on
(one tangent: I wonder if we should rename this repo to go-multiformat. There was pretty resounding agreement that the word multicodec may be confusingly used to describe too many things the last time the IPLD team talked about this. But I also don't feel the need to hold up any other discussion this / if we do engage this, let's spawn another issue for that conversation.)
Strong disagreement. These are called multicodecs and the only confusion in IPLD is that IPLD also has "codecs" (which should probably be called formats". We could call them something else (multicodes?) but it's likely too late to change them and go-multiformats
would imply that this library contains all the multiformats projects.
would strongly suggest not having #136 block on this.
I think there's a minimal subset of this issue that needs to be resolved before closing #136, or at least before doing a release of go-multihash containing #136.
For example, if we're not going to be putting DefaultLengths
for hash functions into the code table (which seems like a thing we shouldn't need to do as the spec behind the hash function already conveys that information) then the Register
function needs to be augmented in #136 or we'll have to perform another breaking change. Similarly, we need some direction of what's going to happen with the Codes
and Names
maps. For example, if the plan is to not include names in the Register
function, but also keep those maps around in the short term then Register
should probably return an error (or panic I guess) if the name is undefined.
go-multicodec contains all the indicator codes -- not just the ones that indicate hashes! Should we... try to do something about that, to attempt to make things carry more meaningful suggestive power?
Probably, it seems to me at least somewhat desirable to make more information from the code table easier for developers to access than searching through a CSV (e.g. through their development tools). Thankfully we already have the Tag
field in the CSV the can be used to autogenerate things here. An example codegen scheme might look like:
Input: (Name, Tag, Code, Description) ->
package {Tag}
type Code uint64 // hopefully we'll never have to exceed this since it's technically a varint 😄
// {Name} has description: {Description}
const {Name} Code = {Code}
Note: we could also duplicate the code in a generic list of all codes and/or supply maps for convenience of anyone who wants to iterate through the codes.
would we actually remove the current constants?
I have no particularly strong preferences here. I get not wanting to break things when there's a totally plausible alternative where things don't break for users (e.g. just marking multicode constants as deprecated and forwarding them to the correct package). However, if a plausible and easy enough to use alternative is available pushing people in the right direction might be worthwhile. I also want to point out that breaking things might be a good idea in the event people were for some reason relying on bad behaviors (e.g. assuming that all valid multihash codes MUST be in the Names/Codes maps and that therefore failing a map check means the code number isn't a hash - like if someone manually made a copy of the ValidCode function).
about whether go-multicodec repo name might itself be confusing -- if we ever want to address that, best to do it immediately before spreading more usage of it.
Maybe, although no strong preferences from me at the moment on the naming. I get @vmx's previous suggestion of referring to items in the table as "multicodes". I think this brings us to then describing a particular code as an "x multicode", where x is effectively the tag in the multicode table (e.g. namespace, multihash, ipld, etc.). I'm not sure if those tags are totally accurate (e.g. are they "hash" codes or "multihash" codes, should the CID codes be tagged the same way as the IPLD codec codes, etc.), but perhaps it's a start 🤷.
For example, if we're not going to be putting DefaultLengths for hash functions into the code table (which seems like a thing we shouldn't need to do as the spec behind the hash function already conveys that information) then the Register function needs to be augmented in #136 or we'll have to perform another breaking change. Similarly, we need some direction of what's going to happen with the Codes and Names maps. For example, if the plan is to not include names in the Register function, but also keep those maps around in the short term then Register should probably return an error (or panic I guess) if the name is undefined.
I believe Hash.Size()
covers this, right?
@Stebalien Ya, I think you're right. Will comment in the PR
We need to talk about how this library approaches constants.
Currently:
s/-/_/g
of the names in the multiformats table...Codes = map[uint64]string
.Names = map[string]uint64
, which is the dual ofCodes
.... so, there are numerous consistency issues with the current status quo, and it's very manual to update.
And we have a recently born go-multicodec repo, which contains a bunch of constants!
go-multiformat
. There was pretty resounding agreement that the word multicodec may be confusingly used to describe too many things the last time the IPLD team talked about this. But I also don't feel the need to hold up any other discussion this / if we do engage this, let's spawn another issue for that conversation.)And, these questions are all raised afresh by #136 -- wherein we've gotta use something for constants in various places; and the revisitation of the registration features means it's a very good time to check in on where we expect any of these mappings to string names to be handled.
Okay, that's all the background. What shall we do? Discuss.