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

feat: add codes as constants #35

Merged
merged 2 commits into from
Jan 24, 2019
Merged

feat: add codes as constants #35

merged 2 commits into from
Jan 24, 2019

Conversation

vmx
Copy link
Member

@vmx vmx commented Jan 8, 2019

It's now possible to access the codes from the multicodec table
as numbers. This way you can use those new constants instead of
passing strings around.

The constant names are generated automatically from the codec name.
Dashed become underscores, all letters are uppercased. This means
that e.g. the constant for dag-cbor is called multicodec.DAG_CBOR.

If you want to get the human readable name, e.g. for error messages
you can use the constants in multicodec.print. To print dag-cbor
you would then use multicodec.print[multicodec.DAG_CBOR].

Another benefit is that it will become easier to add your own custom
codec. If you want to add your own codec to IPLD or IPFS, you need
to register your string in this module. This means that for trying
things out you'd need to fork this module. If the IPLD and IPFS
code is using those constants instead, you could just pass on your
own constant. IPLD and IPFS only need to be able to accept unknown
codecs, they don't need to understand them.

I'd like to get feedback on this change from the whole @ipfs/javascript-team. As the new constants (ideally) would be adapted across all the IPFS code base.

I'm also unsure about the name print, in case someone has a better name, let me know.

It's now possible to access the codes from the multicodec table
as numbers. This way you can use those new constants instead of
passing strings around.

The constant names are generated automatically from the codec name.
Dashed become underscores, all letters are uppercased. This means
that e.g. the constant for `dag-cbor` is called `multicodec.DAG_CBOR`.

If you want to get the human readable name, e.g. for error messages
you can use the constants in `multicodec.print`. To print `dag-cbor`
you would then use `multicodec.print[multicodec.DAG_CBOR]`.

Another benefit is that it will become easier to add your own custom
codec. If you want to add your own codec to IPLD or IPFS, you need
to register your string in this module. This means that for trying
things out you'd need to fork this module. If the IPLD and IPFS
code is using those constants instead, you could just pass on your
own constant. IPLD and IPFS only need to be able to accept unknown
codecs, they don't need to understand them.
@ghost ghost assigned vmx Jan 8, 2019
@ghost ghost added the status/in-progress In progress label Jan 8, 2019
@hacdias
Copy link
Member

hacdias commented Jan 8, 2019

@vmx I could merge this and release a new version. Or do you think we should wait for more feedback?

@vmx
Copy link
Member Author

vmx commented Jan 8, 2019

I'd like to wait for more feedback. Especially from @alanshaw and @achingbrain (perhaps even @mikeal has an opinion).

@achingbrain
Copy link
Member

I think there needs to be an associated PR to multiformats/js-cid before releasing this, otherwise this line will explode as the CID constructor just assigns the second passed argument to this.codec if the first argument is not a CID, a string or a buffer.

@alanshaw
Copy link
Member

I've registered my feeling on this here ipfs/js-ipfs#1809 (comment)

@vmx
Copy link
Member Author

vmx commented Jan 18, 2019

I think there needs to be an associated PR to multiformats/js-cid before releasing this, otherwise this line will explode as the CID constructor just assigns the second passed argument to this.codec if the first argument is not a CID, a string or a buffer.

It surely makes sense. But it's not necessarily needed for merging. You just need to make sure you don't pass a number into the CID.

Update: I forgot to mention that I've an js-cid change in the pipe, I just want to let it age a bit more to see how well it works when it is actually used.

@vmx
Copy link
Member Author

vmx commented Jan 18, 2019

I've added two convenience methods for making using the constants nicer.

It's not possible to get the varint encoding from a code as well
es getting the code from a varint encoded array-like structure.
@vmx
Copy link
Member Author

vmx commented Jan 18, 2019

Although I normally prefer having APIs where there's only a single way of doing things I think the way to move forward is to merge this PR. It is not a breaking change, all the current code will just keep working. Though new code can use constants instead (which I'll do in the IPLD APIs).

I'd prefer deprecating using strings in the future, but that also isn't a must and might not even happen in the near future.

Before this can be merged, I'd like to get approval from everyone who had concerns (@achingbrain and @alanshaw).

@vmx
Copy link
Member Author

vmx commented Jan 23, 2019

The corresponding js-cid change: multiformats/js-cid#72

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM

@vmx
Copy link
Member Author

vmx commented Jan 24, 2019

@hacdias Yay, I think it's ready to get merged. May I suggest that after merging this one, that you run the script to update the tables before you do a new minor release?

@hacdias hacdias merged commit f2716c6 into master Jan 24, 2019
@hacdias hacdias deleted the new-constants branch January 24, 2019 20:36
@ghost ghost removed the status/in-progress In progress label Jan 24, 2019
@hacdias
Copy link
Member

hacdias commented Jan 24, 2019

@vmx! Done 0.5.0 is out!

@vmx
Copy link
Member Author

vmx commented Jan 24, 2019

Thanks a lot!

PS: Stupid me, it could've been a patch release. I actually tried hard to not making breaking changes during my new IPLD API work. Anyway, it's a bug change though :)

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.

5 participants