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

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

Open
djahandarie opened this issue Mar 16, 2022 · 2 comments · May be fixed by #39
Open

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

djahandarie opened this issue Mar 16, 2022 · 2 comments · May be fixed by #39

Comments

@djahandarie
Copy link

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
Copy link
Owner

bparli commented Mar 20, 2022

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
Copy link
Contributor

rcloran commented Sep 7, 2023

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.

@rcloran rcloran linked a pull request Sep 13, 2023 that will close this issue
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

Successfully merging a pull request may close this issue.

3 participants