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

Don't return both Ipv4Addr and u32 #15

Closed
faern opened this issue Jun 30, 2016 · 2 comments
Closed

Don't return both Ipv4Addr and u32 #15

faern opened this issue Jun 30, 2016 · 2 comments

Comments

@faern
Copy link
Contributor

faern commented Jun 30, 2016

I know I said earlier that I didn't have any more changes before a new release. But after some thinking I realized one thing. What is the reason for returning the tuple (Ipv4Addr, u32) from mask, network and broadcast? I can't see a reason when a caller would want both. Plus, the caller can always go from one to the other very simply. Returning both feels a bit redundant.

I suggest the public API always return the Ipv4Addr version only and internal functions who need the integer representation simply use u32::from internally. Cleaner API plus makes it easier to cast the value. Today if you want the representation in a u64 for example you need to do it in two rows, but with the proposed change it would be possible to do it in one:

let n = u32::from(cidr.mask()) as u64

I didn't submit this as a PR directly since I didn't know if there was any special reason it was designed like this or not. Also it would require a major version bump (0.9.0?). However, it's no hurry to get any of this out. As I will continue to use this library I will probably find more stuff to implement in it in the coming days.

@faern
Copy link
Contributor Author

faern commented Jun 30, 2016

I could not stop myself from trying it out 😉 If you want to see how it turned out: https://github.com/faern/ipnetwork/blob/one-return-value/src/ipv4.rs

@achanda

@faern faern mentioned this issue Jul 4, 2016
@achanda
Copy link
Owner

achanda commented Jul 4, 2016

Closed by #16

@achanda achanda closed this as completed Jul 4, 2016
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