JordanMilne / Advocate

An SSRF-preventing wrapper around Python's requests library. Advocate is no longer maintained, please fork and rename if you would like to continue work on it.
Other
92 stars 17 forks source link

Unvendor ipaddress module (Closes #9) #16

Closed ColdHeat closed 3 years ago

ColdHeat commented 3 years ago

This shouldn't break Py2 compatibility but I had some trouble getting tests running so I'm not 100% sure but I imagine it's fine. Also not sure if Travis still has a free tier?

ColdHeat commented 3 years ago

If it's a concern, this commit appears to be the only new relevant one since the py2-ipaddress module was vendored: https://github.com/kwi-dk/py2-ipaddress/commit/117fc9bbcac487b7f781841a7b6c5c6bfaa867ef

codecov[bot] commented 3 years ago

Codecov Report

Merging #16 (fe01a41) into master (6d699ae) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #16   +/-   ##
=======================================
  Coverage   96.73%   96.73%           
=======================================
  Files           9        9           
  Lines         398      398           
=======================================
  Hits          385      385           
  Misses         13       13           
Impacted Files Coverage Δ
advocate/addrvalidator.py 96.09% <100.00%> (ø)
advocate/connection.py 94.25% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6d699ae...fe01a41. Read the comment docs.

ColdHeat commented 3 years ago

@JordanMilne, tests seem to pass on all supported versions. Seems safe to merge and release as v1.1.0?

JordanMilne commented 3 years ago

Thanks for this, this is great!

@spladug Do you think I should switch this to use https://github.com/phihag/ipaddress at the same time? Py2 code would have to be changed to use unicode strings, but the package is already in r2's requirements. The previously-vendored py2-ipaddress also provides ipaddress, so it might conflict if I don't switch.

spladug commented 3 years ago

That'd probably work! Alternatively, I think it'd be reasonable to drop Py2 support here and if really necessary keep it around on a branch only. That'd probably simplify maintenance quite a bit.

ColdHeat commented 3 years ago

Not sure what the benefit would be of switching to https://github.com/phihag/ipaddress? Maybe just b/c that library is closer to the actual one in Python 3?

Both libraries seem to create an ipaddress module. py2-ipaddress won't even install for me on 3.x. ipaddress (the pip one), you can't import instead of the built-in one in Python 3.

If internally you're using all Python 3 then perhaps just drop Python 2 support entirely and release 2.0.0?

ColdHeat commented 3 years ago

@JordanMilne any chance this can be merged? Would like to use this through PyPI if possible.