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

Removing dead code from nacaddr.py. #275

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Removing dead code from nacaddr.py. #275

wants to merge 3 commits into from

Conversation

ankenyr
Copy link
Collaborator

@ankenyr ankenyr commented Mar 22, 2023

Back in when this project was first created Python had no ipaddress library. IIRC Peter Moody created it and open sourced it which is why you will find it copyrighted by Google in https://github.com/python/cpython/blob/main/Lib/ipaddress.py#L1276

You can see there are some slight differences between the one in the "third party" folder and the open source version which is what these deleted lined helped to bridge.
google/capirca@4f694aa

I also deleted a function _is_subnet_of that was necessary while Google was stuck in pre 3.7 but is included at 3.7. We only support 3.7+ so it is safe to remove _is_subnet_of.

@ankenyr ankenyr requested a review from jtwb March 22, 2023 06:25
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Patch coverage: 65.21% and project coverage change: +0.01 🎉

Comparison is base (98383cf) 91.12% compared to head (afbb11f) 91.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #275      +/-   ##
==========================================
+ Coverage   91.12%   91.14%   +0.01%     
==========================================
  Files          98       98              
  Lines       22164    22151      -13     
  Branches     3320     3319       -1     
==========================================
- Hits        20198    20189       -9     
+ Misses       1279     1276       -3     
+ Partials      687      686       -1     
Flag Coverage Δ
tests 91.09% <65.21%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aerleon/lib/cisco.py 88.70% <0.00%> (ø)
aerleon/lib/ciscoasa.py 57.64% <0.00%> (ø)
aerleon/lib/ipset.py 95.40% <0.00%> (ø)
aerleon/lib/nsxv.py 88.35% <0.00%> (ø)
aerleon/lib/iptables.py 90.59% <75.00%> (ø)
aerleon/lib/nacaddr.py 92.73% <100.00%> (+1.54%) ⬆️
tests/lib/nacaddr_test.py 98.24% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -516,12 +492,5 @@ def AddressListExclude(
return sorted(set(ret_array + superset))


ExcludeAddrs = AddressListExclude
Copy link
Member

Choose a reason for hiding this comment

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

Seems like third party generators could still be using ExcludeAddrs and Supernet, especially if they looked at code in the built-in generators using them. A deprecation notice seems warranted.

Copy link
Member

Choose a reason for hiding this comment

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

I should say, a deprecation notice would be warranted and we should not remove ExcludeAddrs and Supernet right away.

Copy link
Member

@jtwb jtwb left a comment

Choose a reason for hiding this comment

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

See deprecation notice comment.

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.

None yet

2 participants