parse_ip_network now accepts new HOSTMASK flag#376
parse_ip_network now accepts new HOSTMASK flag#376claytonsingh wants to merge 2 commits intonetaddr:masterfrom
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 85.72% 86.01% +0.28%
==========================================
Files 47 47
Lines 4379 4390 +11
Branches 737 740 +3
==========================================
+ Hits 3754 3776 +22
+ Misses 449 436 -13
- Partials 176 178 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you for this. FWIW as I mentioned in #123 (comment) I'm currently undecided on this feature. |
|
Flags in python never quite felt right to me either. I tried to be as consistant with the existing codebase as possible, such as the Otherwise I could reimpliment this with a tuple flags, seperate parameter, a class method, or some other way if you have any ideas. A few interface examples: nwk = IPNetwork('192.168.0.1/24', flags=NOHOST | HOSTMASK)
nwk = IPNetwork('192.168.0.1/24', flags=(NOHOST, HOSTMASK))
nwk = IPNetwork('192.168.0.1/24', nohost=True, hostmask=True)If you feel this is a bad fit I understand you closing the MR. |
|
To clarify, I'm hesitant regarding the feature itself (supporting ACL masks) – the way to trigger the feature is secondary, although I share your thoughts about integer flags here. |
|
Unfortunately cisco did decide to use acl masks, so they are out there and likely never going away. I am currently splitting off the ACL masks then applying a hack before passing it to the IPNetwork constructor. Its works but is kind of a pain. Could you provide more detail as to why you are hesitant regarding the feature itself? |
|
I'm just unsure if the benefit is worth the increased interface complexity cost. Maybe this is used/needed more commonly then I think though. |
|
Interface-wise I think something like
( would be better than the current flag system. |
|
If that is the case then I can (with your approval)
|
|
I think let's hold on on changing the existing flags – I need to think about this some more. This PR though – please change the flag to a keyword-only parameter. One note – we definitely need a doctest in Thank you. |
|
I am going to have to strongly disagree here. I think it would be a really bad idea to split the API and have some flags and some keyword-only parameters. There should be one clear consistant way to call the methods. Edit: There is already a doctest using flags. I could add a note about Cisco and ACLs. |
Resolves #123