multiformats / py-multiaddr

multiaddr implementation in Python
https://multiformats.io/
Other
33 stars 23 forks source link

Allow parsing of DNS names and non-ASCII unix sockets #35

Closed ntninja closed 5 years ago

ntninja commented 5 years ago

Some Details:

I'll probably be back with more patches if this one goes through.

Can I also backport this to Python 2, btw?

codecov[bot] commented 5 years ago

Codecov Report

Merging #35 into master will increase coverage by 0.03%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   97.91%   97.95%   +0.03%     
==========================================
  Files           9        9              
  Lines         671      684      +13     
  Branches       74       76       +2     
==========================================
+ Hits          657      670      +13     
  Misses         11       11              
  Partials        3        3
ntninja commented 5 years ago

@mhchia: Thanks! Regarding Python 2: I'd be willing to backport this on top of the previous (non-Python 3) codebase if you would then release the result as the final Python 2 version (maybe with a FutureWarning regarding further updates or something). The reason I want this is because I'd like to use multiaddresses in the next version of py-ipfs-http-client (previously py-ipfs-api) as the addressing scheme for describing the daemon location, but my library will have to support Python 2 until 31.12.2019 because of an existing Python 2 userbase (stats say about 10% of downloads). Without DNS-names the library is unfortunately not fit for this purpose however, hence me asking.

mhchia commented 5 years ago

@alexander255 IMO it makes sense to support python 2.7 until 2020. But the currently supported versions are python 3.4+, I'm not sure if we have the bandwidth to maintain python 2.7. (At least for myself, I might not be able to help with that.) Since you would like to do the backporting, it sounds good to me. Would you like to do it in this same PR or another? And for the release things, cc @zixuanzh @sbuss . Thank you!

ntninja commented 5 years ago

@mhchia: Do I read this correctly, that you'll refrain from dropping Python 2.7 if I help out on that on an as-needed basis? I'll be happy to do that, but you'll have to ping me when stuff breaks, so that I can fix it. :slightly_smiling_face:

As for implementation of this: I'll open a new PR that partially reverts 164dcc3e17f1393ea9cd64fd252271be5412e78d, but marks Python 2.7 as Allowed to Fail. This way people can contribute new things without worrying about Python 2 too much and I can come in and clean up after them later if they don't want to fix it themselves. After this I'll rebase this PR on top of the new code. Sound good?

mhchia commented 5 years ago

@alexander255

Do I read this correctly, that you'll refrain from dropping Python 2.7 if I help out on that on an as-needed basis? I'll be happy to do that, but you'll have to ping me when stuff breaks, so that I can fix it. 🙂

Yes, exactly. We might need your help if things are broken in Python 2.7.

As for implementation of this: I'll open a new PR that partially reverts 164dcc3, but marks Python 2.7 as Allowed to Fail. This way people can contribute new things without worrying about Python 2 too much and I can come in and clean up after them later if they don't want to fix it themselves. After this I'll rebase this PR on top of the new code. Sound good?

Sounds good to me! I am merging this one and wait for your next PR. Thank you for your efforts:)