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

Set difference of two IpNetworks #125

Open
jvff opened this issue Jan 30, 2020 · 6 comments
Open

Set difference of two IpNetworks #125

jvff opened this issue Jan 30, 2020 · 6 comments

Comments

@jvff
Copy link

jvff commented Jan 30, 2020

We had a need to calculate the set difference between two IpNetworks. So given an initial broad IpNetwork, we wanted to remove a smaller IpNetwork sub-network from it. The operation returned an iterator over the resulting set of IpNetworks.

We have code for this implemented, but we were wondering if this feature is something that would be worthwhile upstreaming. Should I open a PR with the code?

@achanda
Copy link
Owner

achanda commented Jan 30, 2020

Yes, this is definitely something that is useful for the broader community. PR is welcome, thanks!

@jvff
Copy link
Author

jvff commented Jan 31, 2020

Great, thanks! I haven't opened a PR yet because I'm not sure how to proceed. The initial implementation I had was very close to this one, except it didn't actually implement Sub for the IP network types, only the custom IpNetworkSub trait. I can also remove the Sub implementation and just use the custom trait, not sure if having the - operator working with these semantics makes sense.

The idea for this implementation was to reduce the repeated code at the cost of more abstractions and boilerplate code. The goal was to keep the actual operation in one location, so that it's easier to review and to make changes in the future. However, it has the major downside of actually exporting those new abstractions :/

Another solution would be to simplify it by removing the abstractions and simply copying the code. It's duplicated, but it does become simpler.

Let me know which solution you'd prefer, and I can open a PR to start discussing any changes that should be made :)

@jvff
Copy link
Author

jvff commented Feb 12, 2020

Sorry for sending out a large code dump 😅 Would it be okay if I opened a PR using the second solution, which should be simpler?

@achanda
Copy link
Owner

achanda commented Feb 13, 2020

Sorry for being late @jvff and thanks for your work on this. I feel like the second option is more readable and maintainable than the first one. Let me know if you agree. Please feel free to open a PR if you have time to get to it.

@jvff
Copy link
Author

jvff commented Feb 15, 2020

No worries! Thanks for your time on this :)

I've opened a PR (#126) with the implementation. Should this issue be closed and the discussion and review continue there?

@CameronNemo
Copy link

You may be interested in "Radix" trees.

https://github.com/deepfield/py-radix/blob/master/radix.c

https://github.com/ibmandura/ArtTree

I am looking to use them with IP prefixes, and the status quo is an old C library from 20 years ago. (seriously there are go, perl, ruby, python bindings for this little BSD-4-Clause licensed library).

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

No branches or pull requests

3 participants