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

Consider removing From<IpXXAddr> for IpXXNetwork #190

Open
sunshowers opened this issue Apr 30, 2024 · 2 comments
Open

Consider removing From<IpXXAddr> for IpXXNetwork #190

sunshowers opened this issue Apr 30, 2024 · 2 comments

Comments

@sunshowers
Copy link

Hi there, thanks for maintaining this crate!

At our workplace we ran into an issue where we were accidentally converting an Ipv6Addr to an Ipv6Network. The conversion seems a bit risky because an address and a network represent different things.

In contrast, From<Ipv4Network> for IpNetwork and From<Ipv6Network> for IpNetwork are reasonable to have.

Would you consider removing these impls and maybe introducing single constructors to represent the single IP use case? That would have made this bug much easier to spot.

Thanks!

@sunshowers sunshowers changed the title Consider removing From<IpXXAddr> for From<IpXXNetwork> Consider removing From<IpXXAddr> for IpXXNetwork Apr 30, 2024
@achanda
Copy link
Owner

achanda commented Apr 30, 2024

Hi 👋 sounds like you want a conversion from Ipv6Addr to Ipv6Network to error out, but that does not happen due to this impl https://github.com/achanda/ipnetwork/blob/master/src/ipv6.rs#L315 so you want to remove those impls. Is that correct? What would the type signature of the single constructor be?

@sunshowers
Copy link
Author

sunshowers commented Apr 30, 2024

Thanks for the quick response! Yes, I'm suggesting removing those impls. It'll be a breaking change, but I think one that's good from an API misuse perspective.

How about just an Ipv6Network::single(addr: Ipv6Addr) constructor? I'm not saying don't provide this, I'm just saying that it would be nice for that to be more explicit.

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

2 participants