bparli / fastping-rs

ICMP ping library in Rust inspired by go-fastping and AnyEvent::FastPing Perl module
MIT License
78 stars 12 forks source link

(Also?) expose IpAddr in the API directly #30

Open djahandarie opened 2 years ago

djahandarie commented 2 years ago

Currently the API only accepts &strs, but that causes some ineffiencies in caller code if

  1. you are already working with IpAddrs at the callsite, or
  2. you want to process the results (which are IpAddr) and look up some things at the callsite which are key'd on IPs, at which point you'd need to convert the results back to strs, or convert the IP keys at the callsite to IpAddr, doubling the amount of work either way.
bparli commented 2 years ago

Hi @djahandarie - I take your point but a little leary of this one. At first glance, I'm not sure how to do it in an intuitive way that won't require a major API change. I'll think on it some more but also open to suggestions

rcloran commented 1 year ago

An example implementation that maintains API compatibility: https://gist.github.com/rcloran/1b252ddcc1a0ee2c4530b217639635b1

There are a few downsides to doing it this way:

Of course, simply adding a new fn add_addr(..., : IpAddr) (note no "ip") or some other name that makes sense would allow keeping the string version around but marked deprecated for a version.

I'd argue for breaking the API. I think it needs to break either as suggested in this issue or as in #34 . There are a couple of other places that I think would benefit from API changes, too.