refactor: split geolocate in three packages #1150
Conversation
bassosimone
changed the title
| @@ -74,8 +75,8 @@ type Config struct { | |||
| UserAgent string | |||
| } | |||
|
|
|||
| // NewTask creates a new instance of Task from config. | |||
| func NewTask(config Config) *Task { | |||
| // NewIplookupTask creates a new instance of IpLookupTask from config. | |||
There was a problem hiding this comment.
| // NewIplookupTask creates a new instance of IpLookupTask from config. | |
| // NewGeolocateTask creates a new instance of GeolocateTask from config. |
| @@ -56,7 +56,7 @@ var ( | |||
| } | |||
| ) | |||
|
|
|||
| type ipLookupClient struct { | |||
| type IpLookupClient struct { | |||
There was a problem hiding this comment.
Just name this type Client given that we already have the iplookup prefix. Because of this, iplookup.Client looks more concise and conveys the same information of iplookup.IPLookupClient.
|
|
||
| func (MMDBLookupper) LookupCC(ip string) (string, error) { | ||
| return geoipx.LookupCC(ip) | ||
| } |
There was a problem hiding this comment.
I have reviewed where we use mmdbLookupper in the main branch and it seems we're actually using it for testing. Therefore, I think we should not expose this type publicly and just keep it as a private implementation detail that provides us with the correct behavior when not testing.
We should probably also document mmdbLookupper to explain why it exists.
| @@ -59,7 +60,7 @@ type resolverIPLookupper interface { | |||
| } | |||
|
|
|||
| // Config contains configuration for a geolocate Task. | |||
| type Config struct { | |||
| type GeolocateConfig struct { | |||
There was a problem hiding this comment.
I am wondering the extent to which we need to expose details of how we implement geolocation outside of the engine package. It may be desirable to make all the types we moved here from the former geolocate package private and just accept that they're an implementation detail of a session. What do you reckon?
| @@ -107,9 +106,9 @@ type Task struct { | |||
| } | |||
|
|
|||
| // Run runs the task. | |||
| func (op Task) Run(ctx context.Context) (*Results, error) { | |||
| func (op *GeolocateTask) Run(ctx context.Context) (*GeolocateResults, error) { | |||
There was a problem hiding this comment.
I see you converted from (op Task) to (op *GeolocateTask). This change is ~good because we tend to prefer always using the pointer receiver in OONI. However, I would like you to double check that there is no method that changes any field of GeolocateTask, otherwise we'll see a change in behavior.
| // NewTask creates a new instance of Task from config. | ||
| func NewTask(config Config) *Task { | ||
| // NewIplookupTask creates a new instance of IpLookupTask from config. | ||
| func NewGeolocateTask(config GeolocateConfig) *GeolocateTask { |
There was a problem hiding this comment.
Something to think about: we may want to pass a *GeolocateConfig here (which is what we try to do in many cases to have consistent coding style). However, in such a case, we should refactor the code below such that we always avoid mutating the config structure that was passed to us (for correctness). We may also want to consider checking what the calls of NewGeolocateTask do and, if they always initialize all the fields, then we can just avoid setting default values and make all the fields in the config struct mandatory.
| }, | ||
| return &GeolocateTask{ | ||
| countryLookupper: iplookup.MMDBLookupper{}, | ||
| probeIPLookupper: iplookup.IpLookupClient(config), |
There was a problem hiding this comment.
I think it would be cleaner to have a constructor to which we pass the resolver, the logger, and the user agent, rather than relying on a cast. Because it's just three arguments, I would say we do not need to have a structure and it's just fine to pass the three arguments to the constructor instead.
This also provides us with an opportunity to make the iplookup.Client fields private. (We tend to adopt an approach based on constructors and private fields, which is a bit more verbose than initializing structs, but is also useful in that refactoring is a bit easier to implement.)
| probeIPLookupper: iplookup.IpLookupClient(config), | ||
| probeASNLookupper: iplookup.MMDBLookupper{}, | ||
| resolverASNLookupper: iplookup.MMDBLookupper{}, | ||
| resolverIPLookupper: resolverlookup.NewResolverLookupClient(config.Logger), |
There was a problem hiding this comment.
I think the best name here is resolverlookup.NewClient and the corresponding type should also be named resolverlookup.Client, since we don't need to repeat "ResolverLookup" in the name of the type.
| @@ -1,4 +1,4 @@ | |||
| package geolocate | |||
| package iplookup | |||
There was a problem hiding this comment.
Make sure you add a doc.go file for each new package with minimal package-level documentation, please!
Checklist
Description
This diff divides the
geolocatepackage functionality to separateiplookupandresolverlookuppackages. It also moves the rest of the geolocate package contents tointernal/engine.