Gh 784 review three head #513
Conversation
| ipv4: Vec<CountryBlock>, | ||
| ipv6: Vec<CountryBlock>, | ||
| pub ipv4: Vec<CountryBlock>, | ||
| pub ipv6: Vec<CountryBlock>, |
There was a problem hiding this comment.
The need for this public visibility surprises me a little... can you assure yourself that taking it out would truly break anything somewhere else?
Feels like if this was required it could be pointing to a wrong design (exposing the guts while nobody outside probably really need to know the details.)
| pub fn get_node_location(ip_opt: Option<IpAddr>) -> Option<NodeLocation> { | ||
| pub fn get_node_location( | ||
| ip_opt: Option<IpAddr>, | ||
| country_code_finder: &CountryCodeFinder, |
There was a problem hiding this comment.
I'm not sure where you needed to pull this variable out through the function's interface. As far as I remember you only supply &COUNTRY_CODE_FINDER at all those places. It makes a question if it should be inline or not. I'll leave the decision for you as you understand better the code and you might remember also usages (I assume it'd have been in a test) with a reference to a CountryCodeFinder. The problem is I cannot even imagine where you could and why would you get down to constructing a special CountryCodeFinder of somehow different qualities than what the central, official one offers.
There was a problem hiding this comment.
Thank you, this could go back to use only COUNTRY_CODE_FINDER, because we use everywhere only real data
No description provided.