amstocker / python-multiaddr

Python multiaddr implementation -- https://github.com/jbenet/multiaddr
1 stars 0 forks source link

Code Review #2

Open candeira opened 8 years ago

candeira commented 8 years ago

Hi, amstocker:

I'm doing a code review from the bottom up, so my comments go similarly from the low level upwards into the high-level API.

At a higher API level:

I think py-multiaddr should be structured similarly to py-multihash, according to the ideas I expressed in this comment. Same as multihash values are bytestrings, which you can either encode into different encodings, or decode into a tuple with (code, name, length, digest)... we could a multiaddr/multiaddressing split, where:

Maybe I'm somewhat of an API nazi. But I also think that this kind of uniformity would do users of py-ipfs well. Most programmers would use the Multiaddressing object, and those who don't can import the multiaddr module for lower-level tinkering.

amstocker commented 8 years ago

I think it is a good thing that you are an API nazi because I'm sloppy about it! The only thing the multiaddr library really needs to know is the byte-length corresponding to each protocol code. I agree that for the ipfs protocol in multiaddr, this module should use the multihash(ing) library to properly verify a multihash. I also agree that we should split up the module into low-level and high-level APIs, but I would not call it multiaddressing and might instead just include the MultiAddress class in the multiaddr module.