parse_ip_network now accepts new HOSTMASK flag

[claytonsingh]

Resolves #123

[codecov-commenter]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (625a780) 85.72% compared to head (53b8974) 86.01%.
Report is 11 commits behind head on master.

Files	Patch %	Lines
netaddr/ip/__init__.py	66.66%	2 Missing and 2 partials ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

[jstasiak]

Thank you for this. FWIW as I mentioned in #123 (comment) I'm currently undecided on this feature.

[claytonsingh]

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 IPAddress constructor.

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.

[jstasiak]

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.

[claytonsingh]

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?

[jstasiak]

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.

[jstasiak]

Interface-wise I think something like

nwk = IPNetwork('192.168.0.1/24', nohost=True, hostmask=True)

(nohost and hostmask keyword-only, names to be adjusted as needed)

would be better than the current flag system.

[claytonsingh]

If that is the case then I can (with your approval)

• Make a new MR to add keyword flags to all classes
  • Update the documentation and mark the flags field a deprecated
• Update this MR to do the same (removing the HOSTMASK constant)

[jstasiak]

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 IPNetwork to show how the parameter affects parsing (and maybe mention Cisco and ACLs too).

Thank you.

[claytonsingh]

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.
https://github.com/netaddr/netaddr/pull/376/files#diff-a89567e4b1edf284bb7d536df42bfc0d6ac51b2ac9b86cd3bca746169aab4b82R999-R1002