-
Notifications
You must be signed in to change notification settings - Fork 14
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
Proposal: Make IpInfo
generic over localization provider.
#47
Comments
Thanks for the suggestion @Alextopher. I'll talk to the team about this proposal and see if this is something that we want to add. |
Taking up from here @max-ipinfo. @Alextopher thanks again for all your contributions! My reading here, as you also mention, is that this will be a breaking change for users doing custom localization. Secondly, I suspect this will add extra work for users to achieve the same end. So for these two reasons I doubt that it will be worthwhile for users to migrate. But nevertheless we can evaluate how big of an improvement we can achieve in performance. Do you have any before/after numbers around this? |
I don't have numbers around anymore. Nonetheless they are very large strings and hashmaps that need to be |
Agree with you on the cloning overhead and would have eagerly moved forward with your proposal if we could somehow ensure backwards compatibility for all users. I think we can close this issue for now -- we will keep this idea in mind and revisit it if more users express interest. |
Proposed Change
Create a new abstraction for "localization providers" and make
IpInfo
generic over that type. If the spirit of this change is accepted we can also begin to improve the status-quo regarding excessive use of.clone()
inIpInfo
. This could look like making the following change (I'm working through experiments at https://github.com/Alextopher/ipinfo/tree/memory)Motivation
There are some key API guidelines that inspires this kind of change.
Using a trait and generics here minimizes our assumptions on how localization data can be provided. #42 called for the replacement of remote resources with tables baked into the library. If
IpInfo
was generic then we could support both workflows. C-GENERICIpInfo
only uses shared references to localization data. Because of this we should only require users to provide shared references. Then we can empower users to handle memory management themselves. C-CALLER-CONTROLDownsides
This proposal is suggesting making a breaking change. Using a good default type parameter (
StaticTables
) means anyone who hasn't used localization (and just used..Default::default()
won't have a trouble updating.The text was updated successfully, but these errors were encountered: