rust icon indicating copy to clipboard operation
rust copied to clipboard

Proposal: Make `IpInfo` generic over localization provider.

Open Alextopher opened this issue 2 years ago • 0 comments

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() in IpInfo. This could look like making the following change (I'm working through experiments at https://github.com/Alextopher/ipinfo/tree/memory)

pub trait Localization {
    fn country_name(&self, country_code: &str) -> Option<&str>;
    fn is_eu(&self, country_code: &str) -> bool;
    fn flag(&self, country_code: &str) -> Option<&CountryFlag>;
    fn currency(&self, country_code: &str) -> Option<&CountryCurrency>;
    fn continent(&self, country_code: &str) -> Option<&Continent>;
}

// Where `StaticTables` is a crate provided Localization. It is a good default and improvement on the status-quo 
// See: https://github.com/Alextopher/ipinfo/blob/memory/src/localization.rs#L78
pub struct IpInfo<T: Localization = StaticTables> {
    url: String,
    token: Option<String>,
    client: reqwest::Client,
    cache: LruCache<String, IpDetails>,
    localization: T,
}

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-GENERIC

IpInfo 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-CONTROL

Downsides

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.

Alextopher avatar Dec 22 '23 05:12 Alextopher