Make a multicodec.Registry type available. #172
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
this doesn't help with the need in #169 though? I still can't enumerate what's in a given registry, right? |
aschmahmann
requested changes
May 19, 2021
| // https://github.com/multiformats/multicodec/blob/master/table.csv . | ||
| // You should not use indicator numbers which are not specified in that table | ||
| // (however, there is nothing in this implementation that will attempt to stop you, either; please behave). | ||
| type Registry struct { |
There was a problem hiding this comment.
It seems like we should specify the rules around when you can register encoders/decoders either here or in the LinkSystem constructor.
Similarly, should we be adding a mutex lock around the registry operations?
There was a problem hiding this comment.
I've added a glut of additional documentation about the synchronization (or rather, lack of it, and caller's responsibility).
|
@willscott correct, we'll still need #169, although the functions will end up on the struct instead of (or in addition to) globally. |
warpfork
force-pushed
the
multicodec-registry-type
branch
from
385f4d5
to
194d6c9
Compare
The hope is that this might be helpful if you want to build a multicodec registry other than the global one, but are still buying into the idea of the registry being keyed by indicator numbers. (I'm... not actually sure how useful this is, because I'd think if you're building something other than the default, there's also a good chance you'd want *more* features than a primitive numerical mapping, which are probably going to be related to whatever your application logic in the area is, and therefore not possible for this library to usefully anticipate. But, I dunno. I'm doing this because people are proposing attaching more features to the global, and I'm not comfortable with it, and I'm hoping this will provide a pressure release valve.) The current interfaces are functionally unchanged. The multicodec.Registry type can now be used when constructing a cidlink.LinkSystem. (This saves you from having to write the glue functions to unbox cidlink and then do the LookupEncoder and LookupDecode calls.) For where this relates to LinkSystem, I considered a mechanism like: `func (r *Registry) InstallOn(lsys *ipld.LinkSystem)` ... and probably would've found that a bit cleaner. However, it doesn't jive with the way we've isolated the CID types into a package with a LinkSystem just for them (sigh; that really is the gift of complexity that just keeps giving); you can see how the EncoderChooser and DecoderChooser funcs need a tiny bit of type assertions in order to figure out how to extract the multicodec indicator from the Link/LinkPrototype types? That bit is a bit that we still want to keep cordoned off the rest of the import tree. DefaultRegistry is also now an exported variable, in addition to the functions which already worked with the global data. I probably would've preferred to keep the DefaultRegistry variable unexported, because I can't imagine any good coming of touching it, but the relationship to LinkSystem detailed in the above paragraph requires some access to it.
warpfork
force-pushed
the
multicodec-registry-type
branch
from
194d6c9
to
575054d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The hope is that this might be helpful if you want to build a
multicodec registry other than the global one, but are still buying
into the idea of the registry being keyed by indicator numbers.
(I'm... not actually sure how useful this is, because I'd think if
you're building something other than the default, there's also a good
chance you'd want more features than a primitive numerical mapping,
which are probably going to be related to whatever your application
logic in the area is, and therefore not possible for this library
to usefully anticipate. But, I dunno. I'm doing this because
people are proposing attaching more features to the global, and
I'm not comfortable with it, and I'm hoping this will provide a
pressure release valve.)
The current interfaces are functionally unchanged.
The multicodec.Registry type can now be used when constructing
a cidlink.LinkSystem. (This saves you from having to write the
glue functions to unbox cidlink and then do the LookupEncoder
and LookupDecode calls.)