tidy code generation, add KnownCodes, add Code.Tag method #59
Conversation
A few tags got changed; no noteworthy code changes.
Now that Go 1.17 came out four months ago, we can clean up the go:generate comments. We don't need to worry about supporting Go 1.16, as "go generate" only needs to work for the few maintainers. Fixes #41.
Right now, this is simply backed by a code-generated slice, but the API being a function gives us some wiggle room in the future. The test simply ensures the list is reasonably sane; that it has many codes, no unexpected duplicates, and that a few known ones are present in it. Updates #58.
Fixes #58.
|
Both KnownCodes and the Tag method could use smarter code in the future if the number of multicodecs gets large to a point where we start worrying about binary sizes or memory usage. For the time being, I think a global slice and a big switch are fine. |
| t.Fatalf("expected list to have at least 100 elements") | ||
| } | ||
| seen := map[multicodec.Code]bool{} | ||
| missing := map[multicodec.Code]bool{ |
There was a problem hiding this comment.
this isn't 'missing' so much as 'expected'?
There was a problem hiding this comment.
I actually thought about this for half a minute :) it's "expected" when declared, and "missing" when used at the end of the function. so I found "missing" more intuitive: we declare it, update it via the loop, and are left with... the missing bits.
|
@mvdan Thanks, let's keep this open please until I can test it in the go-ipfs commands. |
Sounds good - no rush from my part. |
| {{- end }} | ||
| } | ||
|
|
||
| func (c Code) Tag() string { |
There was a problem hiding this comment.
Can these tags be declared as constants (similar to the codes) to quickly discover the available/existing ones? (instead of needing to browse the long switch)
There was a problem hiding this comment.
(just a nit, not blocking on my end)
There was a problem hiding this comment.
I'm not sure; in the original issue I pondered whether this should be an enum rather than a string, but I also don't think upstream treats tags as fixed, stable constants. They're just metadata strings, as far as I can tell. I worry that using enums would give the false sense that they're indeed fixed forever, just like the multicodec integers are.
Maybe @rvagg @vmx or @willscott have thoughts.
If we don't want to declare an enum, we could still make the enumeration of known tags public, via something like func KnownTags() string :) I initially thought that would be unnecessary, but I'm happy to add it if go-ipfs needs it. What use case do you have? I thought you'd essentially just query tags and compare them against known ones like ipld.
There was a problem hiding this comment.
Yes, metadata rather than "fixed, stable constants", there's some amount of ambivalence toward the tags because categorisation is a tricky thing in many cases so the edges are squishy (as demonstrated by this serialization->ipld switch). I'd prefer that they not be raised too much in prominence in our APIs.
We're going to switch from a CSV to a more flexible format for input at some point (CSV will still exist, but a JSON can hold much more information), so that will give us additional ability to describe and categorise entries. Perhaps even having multiple tags for individual entries. So for now, don't push "tag" too far.
There was a problem hiding this comment.
Again, this is just a nit, feel free to ignore. The suggested pattern is not having hard-coded strings scattered in a giant switch, but this is generated code from a spec'ed table so it's fine.
There was a problem hiding this comment.
I'm going to keep it as a string for now, then. If the upstream CSV becomes richer, or starts declaring tags as fixed constants, then I'll be more than happy to update the API to reflect that.
|
Three reviews and it works for go-ipfs - merging :) Thanks all. |
|
@schomatis @aschmahmann let me know when you're done integrating with this, and I'll be happy to tag v0.4.0. |
As described in #58 (comment). See the individual commit messages - please do not squash.
The only open question to my mind is how the list of codes has a duplicate - Ipfs and Libp2p are both 0x01a5. Right now, both have constants and entries in KnownCodes, but Ipfs (the deprecated one) is omitted from the Tag method as duplicate switch cases aren't allowed. I guess this makes it consistent that we support both, as using the Tag method on either will work, given they have the same tag attribute.
The alternative would be to consistently omit Ipfs everywhere - its constant and its entry in KnownCodes. I lean towards not doing that given how "deprecated" isn't well defined upstream.
cc @schomatis @aschmahmann